Running a MySQL query with an admin class

Question

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.

Answer 1

I think you started writing the admin class wrong. I'm a beginner at OOP myself, but one of the most usefull tips I've encountered so far is to use your code before you write it. So when you start writing the code, you know how you want to use it instead of writing code and figuring out how to use it later. This might seem silly, but i'll give you an example:

You want to check if someone is an admin. If he is an admin, you want his row. If he isn't, you want to do something else.

If I would build your index.php and I didn't write the admin class yet, I would create an index.php like this:

 $auth_admin = new admin(); $admin_id = $_SESSION['admin_session']; if($admin_info = $auth_admin->get_admin($admin_id)) { // You are an admin, echo your admin ID echo "Welcome. Your admin ID is " .$admin_info['admin_id']; } else { echo "You don't belong here."; } 

We've used a function we didn't write yet. Writing it will be easier, since we already know how we want to use it and how it should behave.

 class admin { private $conn; function __construct() { $database = new database(); $this->conn = $database->connect(); } public function get_admin($id) { $admin = false; $stmt = $this->conn->prepare("SELECT * FROM admin WHERE admin_id=:id"); $stmt->bindValue(':id', $id, PDO::PARAM_STR); if($stmt->execute()) { $admin = $stmt->fetch(); } return $admin; } } 

Answer 2

@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.

Answer 3

Have only one class directly interact with your database. It doesn't make sense for your admin class and it definitely doesn't make sense for your index.php to be executing PDO methods. Think of what a nightmare it would be if you decided to change database drivers to MySQLi or the next greatest thing? You would have to go into all your classes and even your regular scripts to make adjustments! A much better approach is to interact with the DB only from your database class. Put the generic runQuery there.

As much as you can, avoid writing queries in your general scripts (such as index.php). Instead, craft them within methods that live in the class you want info from; methods such as getAdmin() and updateAdmin() would make sense. The reasoning is the same as for my last advice; if in the future you decide to change your DB table structure, or use a different storage mechanism altogether (not MySQL), it would really suck to have to go into all your scripts to change your queries. It would be much easier if your queries are concentrated in a few methods

I don't understand why you didn't combined the two lines below...

$db = $database->connect(); $this->conn = $db; 

Into the following...

$this->conn = $database->connect();