7
\$\begingroup\$

Is this PDO wrapper structure dynamic? Can using self:: rather than $this cause problems?

class Database
extends PDO
{
/*@var  mixed $stmt     Used as a temporary variable to hold queries*/
private $stmt = Null;

/*@var $scopeSelectVar  Used as a temporary variable to hold queries*/
private $scopeSelectVar = Null;

/*@Constructor __construct
 *
 * Database __construct ( mixed $db, [ string $serverHost = Null], [ string $dbName = Null],
 * [ string $dbUser = Null], [ string $password = Null], [ bool $persistent = Null])
 *
 *  @access-public
 */
public function __construct($db,
    $serverHost = Null,
    $dbName = Null, $dbUser = Null,
    $password = Null, $persistent = true)
{
    try
    {
        if(is_array($db))
        {
        /*
        * parent::_construct allows easy instantiation and multiple database connections.
        * @param    mixed $db   When array(), var holds all parameters required to connect.
        *                       When string, var holds the type of DB to connect to. E.g. mysql
        *
        * @param        string $serverHost      the host of the DB connection
        * @param        string $dbName          the name of the database
        * @param        string $dbUser          the respective name of the user
        * @param        string $password        the password of the user connecting
        * @param        bool   $persistent      if true, allows connection to persist through the app
        *
        */
            parent::__construct("{$db['dbType']}:host={$db['Host']};
                        dbname={$db['dbName']}",
                        $db['dbUser'],
                        $db['dbPassKey'],
                        array(
                            PDO::ATTR_PERSISTENT => $persistent,
                        ));

        }//if Var $db == array() then connect using array parameters
        else
        {
            //else connect using all the other function parameters
            parent::__construct("{$db}:host={$serverHost};
                        dbname={$dbName}", $dbUser, $password,
                        array(
                            PDO::ATTR_PERSISTENT => $persistent,
                        ));
        }
    /*Uncomment if you have a proper log class*/
        Log::write('../AppLogs/databaseConn.log', 'Connection
            Established at:');
    }
    catch(PDOException $e)/*catching pdo exception and writing it to a file*/
    {

    /*Uncomment if you have a proper log class*/
        Log::write('../AppLogs/databaseErrors.log',
            'Error:'
            .$e->getMessage());
    }
}


/*
 * @access private
 *
 * void defineParamType(string $val)
 *
 * */
private function defineParamType($val)
{
    /*@param string $val    the update or insert query data passed*/
    /*@return const $param  the param type constant returned from the function*/
    switch($val):

        case (is_int($val)):
            $param = PDO::PARAM_INT;
            break;

        case (is_string($val)):
            $param = PDO::PARAM_STR;
            break;

        case (is_bool($val)):
            $param = PDO::PARAM_BOOL;
            break;

        case (is_Null($val)):
            $param = PDO::PARAM_Null;
            break;

        default:
            $param = Null;
    endswitch;

    return $param;
}


/*
 * @access private
 *
 * void insertPrepare(array $bindData)
 * */
private function insertPrepare($bindData)
{
    /* @param array $bindData       Data to be prepared for binding
     * @return array $insertArray
     * */
    ksort($bindData);

    $insertArray = array(
                'fields' => implode("`,`", array_keys($bindData)),
          'placeholders' => ':'.implode(',:',array_keys($bindData)),
    );

    return $insertArray;
}


/*
 * @access private
 *
 * void updatePrepare(array $bindData)
 * */
private function updatePrepare($bindData)
{
    /*
     * @param array $bindData   Data to be prepared for binding
     * @return string $placeHolders
     * */
    ksort($bindData);

     $placeHolders = Null;
    foreach($bindData as $key => $val)
    {
        $placeHolders .= "`$key`=:$key, ";
    }

    $placeHolders = rtrim($placeHolders, ', ');
    return $placeHolders;
}


private function where($arguments = array(), $joinKeyword = 'AND')
{
    ksort($arguments);

        $whereClause = Null;
    foreach($arguments as $key => $val)
    {
        if(is_int($val))
        {
            $whereClause .= "`$key` = $val {$joinKeyword} ";
        }
        else
        {
            $whereClause .= "`$key` = '$val' {$joinKeyword} ";
        }
    }

    $whereClause = rtrim($whereClause, ' '.$joinKeyword.' ');
    $whereClause = "WHERE {$whereClause}";
    return $whereClause;
}


/*
 * @access public
 *
 * void query(string $sql)
 * */
public function query($sql)
{
    /*
     * @param string $sql       the sql command to execute
     * */
    $this->scopeSelectVar = NULL;
    $this->stmt = parent::query($sql);
}


/*
 * @access public
 *
 * void insertRow(string $tableName, string $bindData)
 *
 * $array = array('field1'=>'field1Value')<-Notice the abscence of ":"
 *      $handler->insertRow('table', $array)
 *
 * */
public function insertRow($tableName, $bindData)
{
    /*
     * @param string $tableName     Name of the table that is inserted into
     * @param   array $bindData     array holding the set of column names
     *                              respective data to be inserted
     * */
    try
    {
        $insertData = self::insertPrepare($bindData);

        $this->stmt = parent::prepare("INSERT INTO
                        `{$tableName}` (`{$insertData['fields']}`)
                        VALUES({$insertData['placeholders']})");

        foreach($bindData as $key => $val)
        {
            $param = self::defineParamType($val);

            $this->stmt->bindValue(":$key", $val, $param);
        }

        $query = $this->stmt->execute();
    }
    catch(PDOException $e)
    {

    }
}


/*
 * @access public
 *
 * void updateRow(string $tableName, array $bindData, string $target)
 *
 * Way of use: to update
 *  $array = array('field1'=>'field1Value')<-Notice the abscence of ":"
 *      $handler->updateRow('table', $array, '`field2`='Val'')
 * */
public function updateRow($tableName, $bindData, $target, 
    $targetClause = 'AND')
{
    /*
     * @param string $tableName     The name of the table you're updating
     * @param array $bindData       array of the values to be inserted.
     *                              includes $_POST and $_GET
     * @param string $target        The exact update target. I.e.
     *                              WHERE id='?'
     * */
    try
    {
        $updateData = self::updatePrepare($bindData);
        if(isset($target))
        {
            $target = self::where($target, $targetClause);
        }
        $this->stmt = parent::prepare("UPDATE {$tableName}
                        SET {$updateData} {$target}");
        foreach($bindData as $key => $val)
        {
            $param = self::defineParamType($val);

            $this->stmt->bindValue(":$key", $val, $param);
        }

        $this->stmt->execute();
    }
    catch(PDOException $e)
    {

    }
}

/*
 * @access public
 *
 * void deleteRow(string $tableName, string $target)
 * */
public function deleteRow($tableName, $target)
{
    /*
     * @param string $tableName table to be deleted from
     * @param string $target  target of the delete query
     * */
    try
    {
        return parent::exec("DELETE FROM {$tableName} WHERE
                                {$target}");
    }
    catch(PDOException $e)
    {

    }
}

/*
 * @access public
 *
 * void selectRow(string $fields, string $tableName, string $target)
 * */
public function selectRow($fields, $tableName, array $target = NULL
    , $targetClause = 'AND')
{
    /*
     * @param  string $fields   the fields of selection. E.g. '`field`,`field2`'...
     * @param  string $tableName The name of the target table
     * */
    if(isset($target))
    {
        $where = self::where($target, $targetClause);
    }else
    {
        $where = Null;
    }
    self::query("SELECT {$fields} FROM {$tableName} {$where}");

}

/*
 * @access public
 *
 * void fetch([string $singleReturn = false], [constant $fetchMode = PDO::FETCH_OBJ])
 * */
public function fetch($singleReturn = false,
    $fetchMode = PDO::FETCH_OBJ)
{
    /*
     * @param string $singleReturn  the name of the "single" field to be fetching
     * @param constant $fetchMode   The fetch mode in which the data recieved will be stored
     * @return mixed    Null when conditions are not met, stdClass(object) or string when
     *                  conditions are met.
     * */
    if(!isset($this->stmt)){return false;}

    if($singleReturn == true)
    {
        if($this->scopeSelectVar == false)
        {
            $this->scopeSelectVar = $this->stmt->fetch($fetchMode);

            if(isset($this->scopeSelectVar->$singleReturn))
            {
                return $this->scopeSelectVar->$singleReturn;
            }else
                return false;
        }
    }else
    {
        $this->scopeSelectVar = $this->stmt->fetch($fetchMode);
        return (isset($this->scopeSelectVar)) ?
            $this->scopeSelectVar: new StdClass;
    }
}

/*
 * @access public
 * 
 * void fetchAll([constant $fetchMode = PDO::FETCH_ASSOC])
 * */
public function fetchAll($fetchMode = PDO::FETCH_ASSOC)
{
    /*
     * @param constant $fetchMode Default is PDO::FETCH_ASSOC the mode of fetching data
     * */
    if(!isset($this->stmt)){return false;}

    $fetchVar = $this->stmt->fetchAll($fetchMode);

    return (!empty($fetchVar)) ? $fetchVar: new StdClass;

}

/*
 * @TODO    set a convenient method to quicly setup nested queries
 * */
public function setSubQuery($target, $subQuery,
    $mysqlSubQueryHandler)
{
    //mysql nested query handler
}

/*
 * @access public
 * 
 * void rowCount()
 * */
public function rowCount()
{
    /*
     * @return numeric $this->stmt->rowCount()
     * */
    if(isset($this->stmt))
    {
        return $this->stmt->rowCount();
    }
}

/*
 * @access public
 * 
 * void lastId()
 * 
 * */
public function lastId()
{
    if(isset($this->stmt))
    {
        return parent::lastInsertId();
    }
}

/*
 * @access public
 * 
 * void truncateTable(string $tableName)
 * */
public function truncateTable($tableName)
{
    /*
     * @param string $tableName     The name of table to be truncated
     * Notice: truncation will reset the table and delete the data
     * */
    return self::query("TRUNCATE TABLE {$tableName}");
    echo "Table {$tableName} Truncated on:".date("d-m-Y h:i:s")
        ."\n";
}

/*
 * @access public
 * void debugDumpParams()
 *
 * */
public function debugDumpParams()
{
    return $this->stmt->debugDumpParams();
}


public function rankTable($tableName, $rankableColumn, 
    $orderByColumn, $ascDesc='DESC', $sqlVarName = 'rankNum')
{
    self::query("SET @{$sqlVarName}:= 0");
    self::query("UPDATE `{$tableName}` SET {$rankableColumn}
        =@{$sqlVarName}:=@{$sqlVarName}+1 
        ORDER BY `{$orderByColumn}` {$ascDesc}");
}


public function 


public function __destruct()
{

}
}

I know I probably shouldn't be extending on PDO, but the entire architecture will be redesigned. I just need to make sure the other functions are somewhat nicely coded.

This is my first heavy OOP code design. Please criticise.

\$\endgroup\$

2 Answers 2

7
\$\begingroup\$

When trying to make things like this, it's amazing how quickly it can become complicated and how many OOP ideas and principles can be involved.

Anyway, there's a lot to cover here, so I'm just going to glaze over a few things and may come back and edit more detail in later.

For a "first heavy OOP code design" this is pretty good (probably better than the typical first real go at it), but there are a few things that stood out:

  • self:: is almost always a bad sign
    • It means that a variable is basically a namespaced global
    • It means that a method has absolutely nothing to do with the state of an object of the class it belongs to. This can make sense in a few situations, but it is a decision that should be carefully considered unless it's one of the obvious uses.
    • Your code is wrong on a technical standpoint because you cannot--well, should not, since PHP is overly lenient--call a non-static method statically. It doesn't make sense. $this and self are not interchangable at all.
  • Your class is MySQL specific
    • Backticks are MySQL specific
    • A few other things that I'm too lazy to look back through the code and refind :)
  • You're mixing concerns with query creation and database connection.
    • prepare, query etc are fairly low level operations
    • creating statements with methods like where is a fairly high level operation
      • If you want a DBAL, create a DBAL. A DBAL should have a connection, not be a connection.
    • People for some reason always want to extend PDO. There are situations where that makes sense, however, 99% of the time, it does not.
      • If you want an SQL helper class, then have a class that uses a PDO instance behind the scenes. Don't couple your helper class and your connection class together.
  • You're abusing exceptions
    • How does a consumer of your class know that a PDOException occurred? You should almost never eat an exception. If your methods fail, the consuming code probably needs to know.
  • Your logging is flawed
    • If you ever find yourself used a class like C::a(); C::b(); or if you ever find yourself creating a new object in a method, it's likely a sign that the dependency should be provided by the caller.
      • Provides flexibility (what if someone wants to have two instances of your class and use two different loggers?)
      • Eases coupling (Database is secretly dependent on PDO -- telling consumers to uncomment the code if they have a logger named Log is not sufficient)
    • Pass in a logger instead that implements a certain interface
      • Allows flexibility like a null logger (a logger that just discards everything) or like having one instance log to a DB table and another to a CSV
    • Depending how low level your class ends up being, a logger may not belong there.
      • What I mean by this is that there's a certain level where logging just doesn't make sense. System calls don't log. Even PDO doesn't log. You should probably handle logging in application-land where the application can have a better feel for how logging should be handled.
  • Your constructor kills flexibility
    • SQL Server or Postgres for example, will have much different DSNs than MySQL
      • in trying to be overly helpful, your constructor has killed this flexibility -- there's a reason PDO takes a DSN and not the params your constructor takes.
    • You address persistence, but what about the other attributes?
  • A few stylist notes (note: these are 100% opinion)
    • It's fairly standard to indent to the first level inside of classes
    • switch: and endswitch; are very rarely used. A lot of people will vehemently argue with me on this one, but I think that the endwhile; endif; and so on constructs are a hideous blotch a on C-esque language.
    • Null is almost always written either NULL or null in PHP.

In short, this is a good start, but you're probably headed down the wrong path with this class. There's a lot of technical and design flaws here that need to be addressed. I would suggest learning the OO syntax a bit more and trying to better familiarise yourself with low level philosophies and ideas of object oriented programming (when I have more time later I'll come back and provide some links).

\$\endgroup\$
1
  • \$\begingroup\$ Thanks A lot! I really am going to take those remarks and work at improving each and everyone of them. Right now, I am reading a book by Hasin Hayder called "Publishing Object Oriented Programming with PHP5". I will work on an overhaul of the entire class, and once done, will gladly edit this, or post a new review :) Thanks again. \$\endgroup\$ Commented Aug 13, 2012 at 5:20
1
\$\begingroup\$

On Extending Classes

As Corbin says. It is rarely necessary to extend a predefined class. Think of it like this. You should only "extend" a class if you want to add functionality to it. Functionality mean a new feature. This does not mean, usually, redefining the entire class. Using a class is not the same as extending it. In this case you want to "use" the class, not "extend" it.

Some things about PHPDoc

I don't know how it is treated in other IDE's, but on netbeans (and I'm pretty sure eclipse) your PHPDoc comments do not show up because they are not in the (proper format)[http://www.phpdoc.org/docs/latest/for-users/anatomy-of-a-docblock.html]. This means the IDE doesn't know what to display when you try to use it, which makes these comments mostly useless. The link should help with any further questions that I don't cover in this section. The way you are currently doing it, it is just treated as a normal comment and therefore ignored. To initiate a PHPDoc comment you MUST use two asterisks /** to open it. A simple search and replace can fix that for you. Also, the "access" parameter is unnecessary, that should be documented somewhere on that site I linked you, but they changed it since last I looked and I'm not sure where it is anymore. I also don't think the "constructor" parameter is used anymore either, but I can't swear to that one.

/**@var mixed $stmt Used as a temporary variable to hold queries*/

/**allows easy instantiation and multiple database connections.
  *
  * @param    mixed $db   When array(), var holds all parameters required to connect.
  *                       When string, var holds the type of DB to connect to. E.g. mysql
  *
  * @param        string $serverHost      the host of the DB connection
  * @param        string $dbName          the name of the database
  * @param        string $dbUser          the respective name of the user
  * @param        string $password        the password of the user connecting
  * @param        bool   $persistent      if true, allows connection to persist through the app
  *
  * Database __construct ( mixed $db, [ string $serverHost = Null], [ string $dbName = Null],
  * [ string $dbUser = Null], [ string $password = Null], [ bool $persistent = Null])
  *
  */

The PHPDoc inside your methods ARE useless. Even if they did have the proper syntax, they would do nothing because they are located in the wrong place and are only making it difficult to read your methods. All of your interior DocBlocks should be combined with their exterior ones. Its also redundant and unnecessary to rewrite parent PHPDoc comments. If the parent class has documentation, the child class inherits that as well as any methods and properties. The only time this becomes an issue is if the child method does something completely different than the parent method, but then you are doing something wrong.

Here's another cool thing about PHP properties/PHPDoc comments. Though this is purely preference, I find it easier to keep my properties together this way :)

private
    /**@var mixed $stmt Used as a temporary variable to hold queries*/
    $stmt = Null,

    /**@var mixed $scopeSelectVar Used as a temporary variable to hold queries*/
    $scopeSelectVar = Null
;

Either way, you are definitely better about your documentation than I am. Keep that up and I'll aspire to be more like you :)

Self vs This

Here's a link to help illustrate the difference between self:: and $this->. I have a hard time explaining this one, otherwise I would normally translate this for you. The more helpful of the answers, in my opinion, is the second one. But the first answer as well as a few others are pretty good too, they just expect you to already understand some of the more advanced OOP concepts. Corbin had a good explanation, but it seemed somewhat vague. No offense to Corbin, he's a good reviewer, but this is a difficult topic to explain (at least for me).

If you take nothing else from that link, just remember self:: is static (as is anything with ::) and $this-> is not. You commonly see this syntax in child classes when overriding parent methods where you use the parent:: to call the parent's version of that method (or property). So the below snippet, and any others like it, should not use self:: because those methods are not static.

$param = self::defineParamType($val);

Comments

Endif, endfor, endforeach, etc... All comments along these lines, at least if you have a common IDE, aren't all that helpful. All IDE's, at least all the good ones, have brace matching highlighting, and usually code folding too. Either of these features is usually enough to find the end of a block of code. The comments typically just add noise. So this comment below is unnecessary. In fact, most comments inside methods should be avoided or only used for development.

//if Var $db == array() then connect using array parameters

Don't Repeat Yourself (DRY)

Along with many other principles, the DRY principle is a key point of OOP. Another good one is the Single Responsibility Principle, which is something Corbin was driving at. The names usually explain the principle fairly well. In this case, don't have repetitious code. Usually this means creating a function or method or loop. But in this case, it means moving things around and assigning new variables or redefining existing ones. So, for example here is your constructor, rewritten to use this principle.

$persistance = array( PDO::ATTR_PERSISTENT => $persistent );
$type = $db;

if( is_array( $db ) ) {
    $type = $db[ 'dbType' ];
    $serverHost = $db[ 'Host' ];
    $dbName = $db[ 'dbName' ];

    $dbUser = $db[ 'dbUser' ];
    $password = $db[ 'dbPassKey' ];
}

$conn = "$type:host=$serverHost;dbname=$dbName";
parent::__construct( $conn, $dbUser, $password, $persistance );

Of course, you could use extract() above to an even better effect, but not many people like extract() or its counterpart compact(). The reasoning being that they are difficult to debug for. I believe that so long as the variables expected are being documented and the source is trusted, you should be fine. So the use of these functions are entirely up to you.

Escaping Variables in Strings

You may have noticed in the above section that I did not define $conn the same way you would have. I removed the braces from inside the strings because they are unnecessary. When declaring simple variables inside a string, it is only necessary to use PHP's escape sequence, "{}", when the variable name is likely to run into another word. For instance:

"{$verb}ing"

The only other time it is necessary to use the escape sequence is when using complicated variables. Such as:

"host={$db[ 'Host' ]}"
//OR
"host={$this->host}"

When in doubt, it wont hurt, but it makes it harder to read. Depending on the implementation it is sometimes just better to manually escape these sequences yourself.

Template Language

PHP is sometimes considered a templating language. One of the features that highlights this is the alternative syntax for its statements. For instance if: endif;. The colon and endif are the alternative syntax. Typically this is seen only in templates. So seeing it inside a class is quite odd, especially when not combined with the other alternatives. You should be consistent, and honestly you should only use this format in templates. It's a stylistic choice, but one most of the community agrees with.

switch( $var ) {
    case 1 :
        //contents
    break;
    //etc...
}

Exception Handling

I'm no expert here, but I believe the proper way to do exception handling is merely to throw the exception if one is found then catch it during implementation. So all of these try/catch blocks are unnecessary and redundant.

Conclusion

It was about at this point that I stopped reading. A lot of it appeared to be repeat issues or things Corbin already covered. As Corbin said, this is a good start. Keep up the good work!

\$\endgroup\$
1
  • \$\begingroup\$ I just need to make clear that the reviewer shouldn't take a look at the entire code and analyze it, just point me to the problems/errors/mistakes standing out :) I must thank you for the comment advice, I just recently began working with phpdoc for starters and moved in to commenting this code... The "Don't Repeat Yourself (DRY)" part is brilliant! I felt stupid there for not thinking of that. And I guess along the lines of OOP I forgot many "basic" principals which shouldn't be left out. Many thanks for the awesome review. I am learning so much in here! \$\endgroup\$ Commented Aug 13, 2012 at 22:58

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.