4
\$\begingroup\$

I'm starting to learn the object oriented aspect of PHP. In this regard I made a small "exercise" of sorts "translating" one of my PHP functions into OO mode of PHP. I'm asking if there are there any recommendations you can give me concerning my predicament.

Example of non-OO PHP:

<?php
function pristup($servername, $username, $password, $dbname, $sql){

    $conn=new mysqli($servername, $username, $password, $dbname);

    if($conn->connect_error){

        die("Neuspela konekcija: ".$conn->connect_error);

    }

    $result = $conn->query($sql);
    if ($result == TRUE) {
    //echo "Uspela konekcija";
} else {
    echo "Neuspešno izvršavanje upita: " . $conn->error;
}
    return $result;

    $conn->close();

}

?>

Example of this done in an object oriented manner:

<?php

class konekcija{

    private $servername;
    private $username;
    private $password;
    private $dbname;
    private $sql;

    //Setter functions
    public function setVal($par1, $par2, $par3, $par4, $par5){ //Setovanje vrednosti za upit.
    $this->servername = $par1;
    $this->username = $par2;
    $this->password = $par3;
    $this->dbname = $par4;
    $this->sql = $par5; 

}

    //Getter functions
    public function getServername() { 
        return $this->servername;
    }

    public function getUsername() { 
        return $this->username;
    }

    public function getPassword() { 
        return $this->password;
    }

    public function getDBname() { 
        return $this->dbname;
    }

    public function getSQL() { 
        return $this->sql;
    }

    //Function that executes query.
    public function pristup($server_name, $user_name, $pass_word, $db_name, $sql_query){

        $conn=new mysqli($server_name, $user_name, $pass_word, $db_name);

        if($conn->connect_error){

            die("Neuspela konekcija: ".$conn->connect_error);

        }

        $result = $conn->query($sql_query);

        if ($result == TRUE){

        echo "Uspela konekcija";

        } 
        else{

                echo "Neuspešno izvršavanje upita: " . $conn->error;

            }

        return $this->$result;

        $conn->close();

    }

}


$kon = new konekcija(); //Creation of an object.

//Setting values.
$kon -> setVal("localhost", "root", "", "test", "CREATE TABLE example(
            id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
            firstname VARCHAR(30) NOT NULL,
            lastname VARCHAR(30) NOT NULL,
            email VARCHAR(50),
            reg_date TIMESTAMP
            )");

//Getting values and inserting them into class method.
$kon -> pristup($kon->getServername(), $kon->getUsername(), $kon->getPassword(), $kon->getDBname(), $kon->getSQL());

?>

This works, and I'm asking if this code is any good. Is there a better/more established way of doing this?

\$\endgroup\$
  • \$\begingroup\$ You don't really need to assign so many methods to assigning things. You won't really ever retrieve those again, so dedicating methods to assigning them is not necessary, I don't think. \$\endgroup\$ – Rasclatt Dec 30 '17 at 6:01
  • \$\begingroup\$ @Rasclatt So one method to rule'em all? \$\endgroup\$ – Veljko Stefanovic Dec 30 '17 at 6:16
  • \$\begingroup\$ Post edited according to advice given by @Rasclatt . \$\endgroup\$ – Veljko Stefanovic Dec 30 '17 at 6:22
  • \$\begingroup\$ I am not sure if I am going to write a distinct answer, so just a few pointers. First of all, you shouldn't write some code, or use some paradigm only for sake of it, just because everyone else is doing it. Your new code should be better than old one. Do you see any benefit in your class? If so, what is it? Regarding the class itself, I've got an article, Your first database wrapper's childhood diseases, it is definitely worth to read \$\endgroup\$ – Your Common Sense Dec 30 '17 at 10:00
  • \$\begingroup\$ Also I wrote an answer to the similar question recently, you may find it useful: codereview.stackexchange.com/questions/183801/… \$\endgroup\$ – Your Common Sense Dec 30 '17 at 10:07
0
\$\begingroup\$

I think you might be better served breaking what you have into a database class and a query-type class that will use the database class to accomplish queries and return rows, maybe something like:

/vendor/MyApp/Database/Model.php

<?php
namespace MyApp\Database;
# You may want to implement an interface here or abstract class to allow for
# different connection types (looking to the future)
class Model
{
    protected $conn,
              $Log = false;
    # I think you might want to pass a logging class to save server/connection errors
    public function __construct(\MyApp\Logging\Model $Log)
    {
        $this->Log = $Log;
    }

    public function createConnection($host,$username,$password,$dbname)
    {
        $this->conn = new mysqli($host, $username, $password, $dbname);
        if(!empty($this->conn->connect_error)) {
            # Here is where you want to log potentially sensitive errors.
            $this->Log->saveError($this->conn->connect_error);
            # Just let the user know something has gone wrong, they don't need
            # to know the error code and cryptic messages doled out by MySQL
            throw new \Exception('Database can not connect.');
        }

        return $this;
    }

    public function getConnection()
    {
        if($this->conn instanceof \MySQLi)
            return $this->conn;

        throw new \Exception('Database connection not yet set.');
    }

    public function closeConnection()
    {
        if($this->conn instanceof \MySQLi)
            $this->conn->close();
    }
}

/vendor/MyApp/Query/Controller.php

<?php
namespace MyApp\Query;
# You can use the database model to create a contained connection
# You could extend this DB class to make this Query class, I am just going
# to use it instead though
use \MyApp\Database\Model as Connection;

class Controller
{
    private $con,
            $query,
            $Log,
            $Db;

    public function __construct(\MyApp\Logging\Model $Log)
    {
        # You probably want to log failed queries, so use the same logging class
        $this->Log = $Log;
        # Pass that class to the connection
        $this->Db = new Connection($this->Log);
        # Create the connection here using the CONSTANTS in your config file
        # Assign the connection internally to this class
        $this->con = $this->Db->createConnection(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_NAME)
            ->getConnection();
    }
    # You probably want to make a way to bind parameters
    # Also note, I use PDO, so is copied from yours...
    public function query($sql)
    {
        $this->query = $this->con->query($sql);
        if($this->query != TRUE) {
            # Same as connection, you need to know what really happened,
            # but your user doesn't
            $this->Log->saveError($conn->error);
            throw new \Exception('An error occurred.');
        }

        return $this;
    }
    # Here is where you would send back the rows (probably requires a while() here)
    public function getResults()
    {
        return $this->query;
    }
    # You may want to be able to get the raw connection, who knows...
    public function getConnection()
    {
        return $this->con;
    }
}

You won't ever (or very rarely) have to retrieve the database credentials so you don't really need to dedicate a bunch of methods to assign those.

Depending on your app, you can do a contained engine without having to always add your db credentials to the main database class:

<?php
# I might do a config file that has this kind of info or an editable php 
# array with the credentials in it. Something that can not be directly 
# accessed by a user
define('DB_HOST','localhost');
define('DB_USERNAME','root');
define('DB_PASSWORD','');
define('DB_NAME','dbname');
# Create the logging class
$Logger = new \MyApp\Logging\Model();
# Create the query engine, add in a logging class to keep track of errors
# You don't want to show the user the actual errors, those are best kept
# for the administrator's eyes. The user just needs to know something is not
# working
# One note here, you would want to pass $qEngine to all classes that use
# the query engine / database connection, don't make more instances of this
$qEngine = new \MyApp\Query\Controller($Logger);
# Create a query and get the results
# This is to allow for writing queries and fetch rows (the getResults() method)
$results = $qEngine->query("SELECT * FROM `users`")->getResults();

Finally, you don't really want to echo anything except in your view so you can use a try to catch errors and print them in the view, not inside the data fetching methods. Depending on the error, die() might be a bit much. If you throw an Exception, depending on the severity of it, you can print a designed page that has your error printed to the page instead of the rather bleak look of a die('String of text here.').

Anyway, I am no hardcore expert, there are lots of books and articles regarding patterns and such that would be helpful to you. These are some things I have done in the past (and still do for the most part). Also note, I haven't tested this specifically, it's more to just give you some ideas.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter \$\endgroup\$ – Your Common Sense Dec 30 '17 at 10:06
  • \$\begingroup\$ Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already? \$\endgroup\$ – Your Common Sense Dec 30 '17 at 10:10
  • \$\begingroup\$ @YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there. \$\endgroup\$ – Rasclatt Dec 30 '17 at 17:06
  • \$\begingroup\$ @YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string. \$\endgroup\$ – Rasclatt Dec 30 '17 at 17:11
  • \$\begingroup\$ I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC. \$\endgroup\$ – Veljko Stefanovic Dec 30 '17 at 19:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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