3
\$\begingroup\$

I just start to learn OOP, and it's far more interesting than procedural style.

I have a complete working online store written in procedural style. After realizing that my code is becoming very huge and very complex, I'm searching for methods to make my code more maintainable and more comfortable. One solution of course is to move to OOP style. But because I just started to understand how OOP work, I do not know which way to go.

con.php:

// Create a database connection
$connection = mysqli_connect(DB_SERVER, DB_USER, DB_PASS, DB_NAME);
// Test if connection occurred.
if(mysqli_connect_errno()) {
    die();
}

qfunctions.php

function get_user_info($user_id) {
// this function make a query for the user info
    global $connection, $query_error;;
    $query  = "SELECT *
                FROM users 
                WHERE users.user_id = {$user_id}";              
    $user_result = mysqli_query($connection, $query);

    // Test if there was a qurey error                                      
    if (!$user_result || mysqli_num_rows($user_result) == 0) {
        $query_error = $err_array["PROFILE_NOT_FOUND"]; 
    } else {
        return $user_result;    
    }   

}

I feel like this method is good but not the most efficient way possible, so I built a class for my database.

new_con.php

class MySQLDatabase {

private $connection;

function __construct() {
    $this->open_connection();
}

public function open_connection() {
    $this->connection = mysqli_connect(DB_SERVER, DB_USER, DB_PASS, DB_NAME);
    if(!$this->connection) {
        die("Database connection failed: " .mysqli_connect_errno());
    } else {
        $db_select = mysqli_select_db($this->connection, DB_NAME);
        if(!$db_select) {
        die("Database selection failed: " .mysqli_error($this->connection));
    } 
    }
}

public function close_connection() {
    if(isset($this->connection)) {
    mysqli_close($this->connection);
    unset($this->connection);
    }
}

public function query($sql) {

    $result = mysqli_query($this->connection, $sql);
    $this->confirm_query($result);
    return $result;
}

public function fetch_assoc($result_set) {
    return mysqli_fetch_assoc($result_set); 
}


private function confirm_query($result) {
    if(!$result) {
        die("Database query failed: " . mysqli_error($this->connection));
    }
}

public function free_result($result) {
    if($result) {
        return mysqli_free_result($result); 
    }
}


}

I think this class is start to make the code more readable, but I now face a new problem: all my query functions are in procedural style and I don't know which way to go.

Option 1: change all the function to OOP style

new_qfunction.php

function get_user_info() {
global $DB;
$sql = "SELECT  * FROM USERS";
$result = $DB->query($sql);
return $DB->fetch_assoc($result);
}

Option 2: change all the functions to class

Class User {
function get_user_info($user_id) {
    $DB = new MySQLDatabase();
    $sql = "SELECT  * FROM USERS";
    $result = $DB->query($sql);
    return $DB->fetch_assoc($result);
}
}

I don't feel like doing Option 1 for all the query function will make a big difference. For Option 2, I think it's better but far complex, especially when I just start coding in OOP.

Can you tell me what is the best approach to move an entire working procedural site to an OOP site, slowly and safety, and without making the code more complex than its now?

Can you recommend an approach to take? And if I go with Option 1, will it be worthwhile?

\$\endgroup\$
0

3 Answers 3

4
\$\begingroup\$

At first I'm not mysqli guy and maybe somewhere I have mistake but the important part here Is the idea.

My advice to you is to throw exception instead of die or exit because you can catch them a lot easier. In your case you can put instantiation of MySQLDatabase in try/catch and if exception is occurred to show static page or whatever you wish.

class MySQLDatabase {

    private $connection;

    function __construct() {
        $this->open_connection();
    }

    public function open_connection() {
        $this->connection = mysqli_connect(DB_SERVER, DB_USER, DB_PASS, DB_NAME);
        if(!$this->connection) {
            throw new \Exception("Database connection failed: " .mysqli_connect_errno());
        }
    }
    /*
     * This function (mysqli_select_db) should only be used to change the default
     * database for the connection. You can select the default database with 4th
     * parameter in mysqli_connect().
     */
    public function select_db($db_name){
        mysqli_select_db($this->connection, $db_name);
        if(!$this->connection){
            throw new \Exception(" meaningfull message");
        }
        return $this;
    }

    public function close_connection() {
        mysqli_close($this->connection);
        unset($this->connection);
    }

    public function query($sql) {
        $result = mysqli_query($this->connection, $sql);
        return $this->confirm_query($result);
    }

    public function fetch_assoc($result_set) {
        return mysqli_fetch_assoc($result_set); 
    }

    private function confirm_query($result) {
        if(!$result) {
            throw new \Exception("Database query failed: " . mysqli_error($this->connection));
        }
        return $result;
    }

    public function free_result($result) {
        if($result) {
            return mysqli_free_result($result); 
        }
    }
}

I will make separate class for every table or group of futures(e.g. some kind of statistics) in separate class and all SQL queries are there. Important part of this is to not make validation in this class. You must consider that every parameter of method Is validated and correct.

class UserDomain
{
    protected $conn;

    public function __construct(MySQLDatabase $conn){
      $this->conn = $conn;
    }

    public function getUser($id){
        $sql = "SELECT * FROM users WHERE u.id = ?";
        $stmt = $this->conn->prepare($sql);
        $stmt->bind_param('i', $id);
        $stmt->execute();
        $tmp = array();
        while($res = $stmt->fetch()){
            $tmp[] = $res;
        }

        return $tmp;
    }

    public function setName($name, $user_id){}
    public function loginBy($user_id, $password){}
    public function changePassword($new_paswwrod, $user_id){}
    public function deleteUser($user_id){}
}

Here we have our class User. It can be tested very easy because your sql queries are now separated

// This class represents single user
class User {
    protected $id;

    protected $db;

    public function __construct($id, UserDomain $conn){
        $this->id = $id;
        $this->conn = $conn;
    }

    function getUserId(){
        return $this->id;
    }

    function changePassword($old_password, $new_password){

        if($this->conn->loginBy($this->id, $password)){
            // here you must validate new password against your business logic and if its true or pass try/catch you will update
            $this->changePassword($new_password, $this->id);
        }
    }

    function getUserInfo(){
        return $this->conn->getUserInfo($this->id);
    }
}

Finally I recommend you to watch this speak from Adam Culp he also have and book I think you will like it. And will give you nice ideas about refactoring legacy code and not write it

\$\endgroup\$
1
  • \$\begingroup\$ Thanks 'Радослав Йорданов', nice thinking, I like your approach. \$\endgroup\$
    – david2000
    Feb 6, 2017 at 21:31
2
\$\begingroup\$

Consider dumping this database wrapper class. It is really not adding value over just directly using mysqli.

Typically one might write a class around an existing DB class or abstraction to do things like:

  • Manage DB connections. Your class doesn't really do this (though it looks like you are trying to create singleton, just not in a proper way)
  • Abstract caller from underlying database implementation. You are not doing that here as your class is leaking underlying mysqli implementation.
  • Provide ORM/"model" behavior for interacting with database.
  • Provide natural language query-building capability.

You are doing none of this and are in fact limiting the things the caller can do with the underlying connection.


This class is not using mysqli in an OO fashion which seems like a strange decision.


This class is outputting directly to standard out on error conditions. Leave end user messaging to logic further up the call that that is better positioned to message the end user in a meaningful way. You should be concerned only about logging errors and/or throwing exceptions from this class, not with outputting to user.


You should familiarize yourself with dependency injection as preffered means to deliver dependencies (such as a database object) to code that needs to use it. This prevents you from having to use patterns like singleton to instantiate these dependencies within classes that need these dependencies.

So for example, rather than singleton pattern like this:

class MysqliSingleton {
    protected static $mysqli;

    protected function __construct() {}

    public static function getMysqli() {
        if(empty(self::$mysqli)) {
            self::$mysqli = new mysqli(...);
        }
        return self::$mysqli;
    }
}

class ClassThatNeedsMysqli
{
    protected $mysqli;

    public function __construct() {
        $this->mysqli = MysqliSingleton::getInstance();
    }
}

You would use something like this:

// somewhere in startup script
$mysqli = new mysqli(...);

// where you instantiate the class that needs mysqli
$obj = new ClassThatNeedsMysqli($mysqli);

// or alternatively to the above approach, you could use a singleton or provider method like the following
// either way, you are passing the dependency to the class that needs it
$object = new ClassThatNeedsMysqli(MysqliProvider::getInstance());

// and your class
class ClassThatNeedsMysqli
{
    protected $mysqli;

    public function __construct(mysqli $mysqli) {
        $this->mysqli = $mysqli;
    }
}

This may seem like a subtle difference, but it is a very powerful approach in that you establish a strong contract that this class must have it's dependency passed to it or it cannot even instantiate . An InvalidArgumentException would be thrown if value passed for $mysqli is not a valid mysqli object.

You also remove all the logic and potential error/exception handling in classes that need such a dependency. Why should all these classes need to know how to instantiate a mysqli object? They should just get a mysqli object in an appropriate state that just works.


Get in the habit of using exact comparisons (===, !==) rather than loose comparisons (==, !=) by default. Loose comparisons lead to buggy code and should be avoided unless that is a specific use case for the loose comparison (and I usually find myself adding a comment in such cases as to why a loose comparison is appropriate)


Get in the habit of using parametrized prepared statements when querying the database in like you search by user id query. Right now, you are seemingly doing nothing to sanitize your parametric values for use in query.


I would suggest you use PDO rather than mysqli if rewriting this. With PDO you can work in exception mode to where you don't have to create your own exception throwing logic around mysqli. It is also provide compatibility across a variety of relational database types.


You are not doing anything on your public class methods or functions to validate data passed to it. For example here:

public function query($sql) {

    $result = mysqli_query($this->connection, $sql);
    $this->confirm_query($result);
    return $result;
}

You can get to the point of making the (relatively) expensive database query even if an empty string is passed to the method. You should get in the habit of failing early and failing loudly if your methods get passed bad data. This makes your application easier to maintain / debug.


I would highly recommend this resource as a good starting point for learning how to properly code in PHP in a professional manner.

http://www.phptherightway.com/

\$\endgroup\$
1
\$\begingroup\$

I would take option two but have your $DB object in the __construct(), like:

<?php 

Class User {
    protected $DB;

    public function __construct(){
        $this->DB = new MySQLDatabase();
    }

    function get_user_info($user_id) {
        $sql = "SELECT  * FROM USERS";
        $result = $this->DB->query($sql);
        return $this->DB->fetch_assoc($result);
    }
}

That way, every time you create a new User() object, it'll create your database instance, so for each function in your class, you won't need:

$DB = new MySQLDatabase();

in every method that you create. Also consider looking into Object Visibility in PHP (public, private and protected).

\$\endgroup\$
1
  • 1
    \$\begingroup\$ This is bad advice. You should learn about dependency injection (and how to avoid high coupling in general). \$\endgroup\$
    – shudder
    Feb 7, 2017 at 10:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.