HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Running a MySQL query with an admin class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
adminwithquerymysqlrunningclass

Problem

This is my admin class, DB class and how I use OOP.

I am looking for ways to improve my code to make better use of OOP. Please help me if you think I can improve my code in some way.

db.class.php

class database
{   
    private $conn;
    private $db_name = 'profil_penduduk';
    private $db_user = 'root';
    private $db_pass = '';
    private $db_host = 'localhost';

    public function connect()
    {

        $this->conn = null;    
        try
        {
            $this->conn = new PDO("mysql:host=$this->db_host;dbname=$this->db_name", $this->db_user, $this->db_pass);
            $this->conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);   
        }
        catch(PDOException $exception)
        {
            echo "Connection error: " . $exception->getMessage();
        }

        return $this->conn;
    }
}


admin.class.php

class admin
{
    private $conn;

    function __construct()
    {
        # code...
        $database = new database();
        $db = $database->connect();
        $this->conn = $db;
    }

    public function runQuery($sql)
    {
        # code...
        $stmt = $this->conn->prepare($sql);
        return $stmt;
    }
}


This is how I fetch the data:

index.php

$auth_admin = new admin();

$admin_id = $_SESSION['admin_session'];

$stmt = $auth_admin->runQuery("SELECT * FROM admin WHERE admin_id=:admin_id");
$stmt->execute(array(":admin_id"=>$admin_id));

$adminRow=$stmt->fetch(PDO::FETCH_ASSOC);


Is there a better way for me to do this? Should I use CRUD? Or should I make a function with this query:

"SELECT * FROM admin WHERE admin_id=:admin_id"


inside the admin class?

Should I also use interface in my class? I don't understand much about interfaces, either.

Solution

@Max already did a good job at describing how you could write better code.

Here is what is wrong with your current approach:

The main problem is that your classes are tightly coupled. All the files you posted need to be aware that you are using PDO, which isn't good (it's hard to read and maintain).

Your admin class also doesn't make any sense. It doesn't contain code that is specifically concerned with anything related to admins, it just contains part of the database access code (but for some odd reason not all of it).

So what I would do now is:

  • keep all the PDO calls in one place. There really isn't a good reason to have prepare in one place, and execute and fetch in a completely different place.



  • remove the generic runQuery method from admin, as it's not specific to admins.



  • move the admin select query into a method called getById inside the admin class.



Doing this, you basically arrive at the code @Max posted.

Misc

  • Class names should be upper-case.



  • Don't echo in classes, it makes them difficult to reuse. Just throw the exception upwards and deal with it later.



  • The way connect is written, you would use a new connection for each object. For example, if you not only had an admin class, but also a SomeData class, it would also call connect, creating a new connection. It's better to reuse the same connection across the request. So either pass the connection to the admin constructor, or to the getById method.



  • Try to be consistent with your names. Examples are admin_session vs admin_id, auth_admin vs admin, or conn vs db. These inconsistencies do decrease readability, as a reader has to think about them each time.



  • Either use camelCase or snake_case, don't mix them.



  • Your spacing is off.

Context

StackExchange Code Review Q#133073, answer score: 2

Revisions (0)

No revisions yet.