Critique first step into OOP?

PHP
1

Hey guys, this being my first real step into OOP (I’ve read tons of stuff on it and I understand most of the concepts, just not when/how I’ll need to use it) so I decided to make a very simple mysql db class, keep in mind it has no security and I usually use PDO, but I did this just for ease of creating the class. Just looking for critique on the actual OOP aspect of it.

class database
{
    public $workingTable = NULL;
    public $workingQuery = NULL;
    public $whereClause  = NULL;


    function __construct($host, $dbname, $dbuser, $dbpass)
    {
        $connection = mysql_connect($host, $dbuser, $dbpass) or die("Error connecting to database user.");
        $database   = mysql_select_db($dbname, $connection) or die("Error connecting to database.");

        echo "Connection to $dbname Successful.<br>";
    }
    
    function setTable($tableName)
    {
        $this->workingTable = $tableName;
        return $this->workingTable;
    }

    function where($field, $value)
    {
        $this->whereClause = "WHERE `$field` = '$value';";
        return $this->whereClause;
    }

    /**
     * Default is SELECT * FROM table - so if you have a workingTable already set
     * You can perform $database->select(); and it will select everything from your current
     * table. This is not considered best practice, so best to individual point out what fields
     * you want to select.
     * @param string $what
     * @param string $tableName
     */
    function select($fields="*", $tableName=null)
    {
        if ($tableName==null)
        {
            $tableName = $this->workingTable;
        } else {
            $this->workingTable = $tableName;
        }

        // So select * is mode 0
        // select `this` from is mode 1
        // select `this`, `this`, `andthis`, is mode 2
        if ($fields=="*")
        {
            $mode=0;
        } else {
            if (!is_array($fields))
            {
                $mode=1;
            } else {
                $mode=2;
            }
        }

        // Start building Query....
        $this->workingQuery = "SELECT ";

        switch ($mode)
        {
            case "0":
                $this->workingQuery .= "*";
                break;

            case "1":
                $this->workingQuery .= "`$fields`";
                break;

            case "2":
                foreach ($fields as $field)
                {
                    $this->workingQuery .= "`$field`,";
                }
                $this->workingQuery = rtrim($this->workingQuery, ",");
                break;
        }

        // add FROM tablename
        $this->workingQuery .= " FROM $tableName;";

        // Implement the where
        if ($this->whereClause != null)
        {
           $this->workingQuery = str_replace(";", " ", $this->workingQuery);
           $this->workingQuery .= $this->whereClause;
        }

        echo $this->workingQuery;



    }


    function insert($fields, $values, $tableName=null)
    {
        if ($tableName==null)
        {
            $tableName = $this->workingTable;
        } else {
            $this->workingTable = $tableName;
        }
        
        $this->workingQuery = "INSERT INTO `".$this->workingTable."` (";

        $mode=0;

        if (is_array($fields))
        {
           $mode=1;
        }

        switch($mode)
        {
            case "0": // one field
                $this->workingQuery .= "`".$fields."`) VALUES ('$values')";
                break;

            case "1": // more than one field
                // add `field`, `field`, `field`,
                foreach ($fields as $field)
                {
                    $this->workingQuery .= "`".$field."`, ";
                }
                // remove trailing ,
                $this->workingQuery = rtrim($this->workingQuery, ", ");
                // add ) VALUES (
                $this->workingQuery .= ") VALUES (";

                foreach ($values as $value)
                {
                    $this->workingQuery .= "'$value', ";
                }
                 // remove trailing ,
                $this->workingQuery = rtrim($this->workingQuery, ", ");
                // add )
                $this->workingQuery .= ");";
                break;
        }

        echo $this->workingQuery;


    }


}

All it has is insert/select and a where clause, so if you set a working table using setTable() that will be the table you’re working on until otherwise noted, you can insert single values, or both arrays of fields and values. You can select specific fields but by default it selects *.

My personal critique would be is I’m pretty sure all the variables at the top can be set to protected correct? Since they wouldn’t be used by the end user just the instance of the class itself? And also, maybe I should have a destruct method closing the mysql connection, and setting all the variables back to null? I guess what I’m saying is if I have an instance of this class and setTable to “users” and then I instantiate the class elsewhere, will it carry that value?

Thanks

2

Protected variables can be accessed only from the class itself and other classes that inherit from it. If you
want the class’s variables not to be accessed or modified from elsewhere you will make them private.

No, if you create another instance of the class it will not carry the values, it is another object with other properties. However you can pass the instance’s variable to functions.

What you have above is a class that ties db functionality with your data structures. I normally create a wrapper class
for the database functions. Example:


public function insert($query)
{
    if (!@mysqli_query($this->conn, $query)) return false; else return mysqli_insert_id($this->conn); 
}

Where the above is called like: $result = $db->insert(“INSERT INTO table VALUES (NULL, ‘value’)”);

3

OOP is probably more about an object’s purpose and the structure of object/class relationships, than how an individual class is written. More importantly though, OOP is only useful when there’s a problem that need solving. You haven’t told us what the problem is this class aims to provide a solution for, so that’s going to make it difficult to critique.

First of all though, as you suspected, those properties should be set to protected. Allowing anyone to modify those values directly opens up a whole can of worms. Using a setter, like you already have for setting the table, is almost always a safer option, as by using a setter, you can ensure that the set value of a valid type, format and value, and can then safely use that property in the rest of your class, resting assured that you won’t encounter something unexpected. I generally only use public properties for allowing boolean values to be set, bit it really depends on the circumstances.

Just a note also (in case you had doubts around when to use ‘protected’ and 'private), you should only declare methods and properties as ‘private’ if you intend for your class to be sub-classed, and have a specific reason to hide those methods and properties from the inheriting class. Those cases are rare, so if in doubt, use ‘protected’ instead of ‘private’ to hide the implementation details of your class, from the rest of your application.

Another thing, why are you returning the table name from setTable()? I would return either a boolean (e.g. true) or nothing at all; in this case, I return nothing at all seeing as though setting a table will always succeed.

Generally speaking, this class doesn’t showcase very good object-orientated design. The worker methods, select(), insert() and where() are just string builders which don’t really interact with the rest of the class at all, or any other object for that matter. You could easily break apart these methods into procedural functions by just adding an additional argument to allow one to specify the table - something which strongly indicates poor OO design.

The truth is though, you can’t have possibly demonstrated a good design, as your example serves no purpose. It doesn’t solve any problem. It’s just code for the sake of code. If you had said “Here’s my class which I plan to use to talk to my database in my next project. I’m hoping it will make doing X easier.”, then I’d be able to point out a whole load of issues with your class, and would be able to offer alternative solutions. But because this class isn’t solving any particular problem (at least not that you’d told us) we can’t possibly determine how well it has been designed.

So the thing you need to take away from this, is that before you cut any code, make sure you clearly understand the problem you’re solving. It’s the only way you can achieve a satisfactory outcome.