2
\$\begingroup\$

I'm creating a database class. It's part academic and part something I may show in an interview.

The purpose of this method is to accept a table name as a parameter, check if the PDO Object has been created (this is in an earlier method), check if the table is in the database as to white list the table for going further in the script, and also to check if the parameter contains any characters that MySQL does not support for unquoted identifiers. If it passes then the table is returned.

<?php
class DatabaseClass{
...
public static function getTable($tablename = ''){
    if(!isset(self::$pdo)){
        trigger_error('Error: '.__CLASS__.'::'.__METHOD__.'() requires a database connection.',E_USER_ERROR);
        return false;
    }
    $whitelist = self::$pdo->query('SHOW TABLES');
    if(!in_array($tablename, $whitelist->fetchAll(PDO::FETCH_ASSOC)) || preg_match('[^0-9,a-z,A-Z$_]',$tablename)){
        /*If tablename is not in the database or if it contains illegal characters*/
        return false;
    }else{
        $tbl = self::$pdo->query('SELECT * FROM `'.$tablename.'`');
        return $tbl->fetchAll(PDO::FETCH_ASSOC);
    }
}
...
\$\endgroup\$
3
\$\begingroup\$

When looking over your code I find several things I would do differently. I may seem harsh in my review, but I have no intention of making this a painful experience. If you have any questions feel free to ask. Without further ado, let's go!

The first thing which comes to my mind is the use of static methods. When working in an object oriented manner I rarely use static methods. When I use them though I ensure they follow this simple rule:

Static methods should be entirely self contained. If a static methods depends on an instance, it has potential side-effects.

This is a personal rule :)

To me this means that static methods should be purely input -> process -> output methods. This also implies that no kind of persistence can be done as it would not be associated with the specific instance the static method is declared in. Even if an instance is provided through its argument list you cannot be sure any persistence wasn't done inside the instance. If you use an object inside the static method it should only exists within the method's scope.

A good example for a static method is when the logic is related. Consider normalization of class names before it's instantiated.

/*
 * An entirely self contained static method. The reason it is
 * declared inside the class is, because the functionality is
 * related to the class.
 */
public function normalizeClassname($classname)
{
    $basename  = ucfirst(basename($classname));
    $namespace = substr($classname, 0, -strlen($basename));

    return str_replace('/', '\\', $namespace) . $basename;
}

Preventing side-effects helps you avoid those moments when a bug has you wondering what the !@*? is going on? (More on this in the linked resource in the link "Global State and Singletons").

Your method depends on an instance of PDO. If you adhere to the rule above this cannot happen inside a static method and it should therefore be changed.

/*
 * The 'static' keyword has been removed.
 */
public function getTable($tablename = '') {...

Then looking over the argument list of the method I find it hard to understand why the table name can be omitted (has a default value)? If you don't provide a table name what is there to search for inside the database? I would change this into a required argument.

/*
 * The default argument value has been removed.
 */
public function getTable($tablename) {...

The next may be a little weird, but try to follow me. But first we need quote, everyone loves quotes!

A method/function should do one thing and do it good.

- Robert C. Martin

Both checking for a valid PDO instance and checking if provided table name exists violates this rule. Your method have 2 concerns as of now (there is actually one more, but more on that later). The issue can easily be solved with 2 possible solutions. Since a PDO instance cannot be created if invalid parameters are provided you can always assume any given PDO instance is valid.

The first solution: I assume an instance of PDO is required by your database class. This would be a perfect opportunity to use dependency injection inside a __construct() method. This removes the requirement of checking if a valid PDO instance was provided to your getTable() method as you can fetch the instance from a class property.

/*
 * Example of constructor injection.
 */
public function __construct(PDO $pdo) 
{
    $this->pdo = $pdo;
}

The second solution: If you do want a __constructor() method or this is the only method to use a PDO instance, you could require the PDO instance in the argument list.

/*
 * Alternative to constructor injection. Argument injection!
 */
public function getTable(PDO $pdo, $tablename) {...

If you decide to follow one of the above solutions you have removed the unrelated concern of checking the validity of the PDO instance from your method.


You then make a white-list by fetching an array of tables inside the selected database and check if your provided table name exists in the array. After this you check if the table name is valid. Checking if the table name is valid should be done before the database query is issued (saving server resources). After all there's no use checking if a table exists when the provided table name is invalid. I would also throw exceptions instead of emitting fatal errors. The end result would be the same, but you would have a nice backtrace with the exception.

Your method should now look like the following:

public function getTable($tablename)
{
    /*
     * I have fixed your regular expression since a delimiter was missing. I
     * have also simplified the expression to be case-insensitive (using the 'i' modifier) and 
     * removed the comma since (as far as I know) isn't a valid table name character.
     */ 
    if(!preg_match('~[^a-z0-9$_]~i',$tablename)) {
        throw new UnexpectedValueException('The provided table name contains invalid characters.');
    }

    /*
     * Notice PDO is now fetched through an instance property and
     * is not static anymore.
     */ 
    $whitelist = $this->pdo->query('SHOW TABLES')->fetchAll(\PDO::Fetch_ASSOC);

    if(!in_array($tablename, $whitelist)){
        return false;
    }

    // Removed the ´else´ block as it was unnecessary.

    $tbl = self::$pdo->query('SELECT * FROM `'.$tablename.'`');
    return $tbl->fetchAll(PDO::FETCH_ASSOC);

}

I would then argue that fetching everything inside the table creates 2 new (major) issues. The first is that it's another concern. As I have understood the method validates that a table exists inside the database. Therefore it should't be concerned with fetching data from it. This could also imply renaming the method to validateTableName() or something like it.

The second being a performance risk. Imagine the provided table stores 1 million rows. Are you sure the database connection allows that long a connection? Does PHP have enough memory dedicated? Is the execution time increased for this (potentially) long running task? Are you aware this halts script execution as long as the query is being processed?

There are properly more concerns when fetching entire tables. I would urge you to consider simply returning TRUE and let other methods deal with fetching data.

If you still want to fetch data you should use a prepared statement instead of interpolating the table name directly into the query string. You may reason that it's always you who decides the table name, but using defense in depth is always better. Malicious users can unfortunately do some scary things.

If you agree with me this far your method should look like this:

/*
 * Changed method name to reflect is new intent.
 */
public function tableExists($tablename)
{
    if(!preg_match('~[^a-z0-9$_]~i',$tablename)) {
        throw new UnexpectedValueException('The provided table name contains invalid characters.');
    }

    $whitelist = $this->pdo->query('SHOW TABLES')->fetchAll(\PDO::Fetch_ASSOC);

    /*
     * Added strict comparison.
     */
    return in_array($tablename, $whitelist, true));
}

The method is now more concise and shows a clear purpose. The readability has also improved by giving more space between the lines. Remember that your code shouldn't be hard to read.

Bonus

Your original method emitted an error using the native trigger_error() function with the E_USER_ERROR flag. This is the same as an exception regarding functionality. But with exceptions you give your program a chance to recover. The calling code or a generic exception handler (set using set_exception_handler()) can then decide if anything can be done before the script fails with a fatal error.

Regarding concerns: I have mentioned this several times and I think this is one of the best things you can make a habit. I have several times when I found a bug praised myself for separating logic into small pieces from the start. This makes correcting the bug easier as you know exactly where the logic in question is defined. This also helps readability when your functions/methods are concise.

The resources linked in this awesome answer on StackOverflow has helped me along the way. Especially the second link about Clean Code III: Functions about function/method concerns is relevant to this question.

Hope this helps, happy coding!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ In your last code-example, the method should've been renamed since it does not 'get' anything, isTablePresent or tableExists or something like this would fit better. Also, you could just return false on line #4 since the whole purpose of this function is to check whether a table exists or not. But thats personal preference i guess. \$\endgroup\$ – tkausl Jul 19 '15 at 6:52
  • \$\begingroup\$ @tkausl - You are right. I guess I had been writing this for so long I missed things, thanks. About renaming the method. I agree with you and hinted at it in text, but felt like it was restructuring the method too much from its original signature. I will update the answer to reflect this better. \$\endgroup\$ – AnotherGuy Jul 19 '15 at 11:59

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.