Static utility functions to turn arrays into SQL statements

Question

I have an object that houses only static functions. The functions are stand alone and are not dependent on the state of the object.

I hear that static functions are a nightmare in terms of testing, an anti-pattern if you will. I get it, the state of the object could be impossible to predict.. But these are stateless.

My question is, Should I place these functions into a class of their own or am I ok by leaving them as static methods?

class Utility {

public static function arrayToInsertStatement($table, array $data) {
    $sql = "INSERT INTO %s (%s) VALUES (:%s)";
    return sprintf($sql, $table, implode(', ', array_keys($data)), implode(', :', array_keys($data)));
}

public static function arrayToSelectStatment($table, array $fields, $whereStatement) {
    $sql = "SELECT %s FROM %s %s";
    if ($whereStatement) {
        $whereStatement = sprintf(" WHERE %s", $whereStatement);
    }
    return sprintf($sql, implode(',', $fields), $table, $whereStatement);
}

public static function arrayToUpdateStatement($table, array $data, $whereStatement = null) {
    $t_sql = "UPDATE %s SET %s%s";
    $sets = self::arrayToKeyedArray($data);
    if ($whereStatement) {
        $whereStatement = sprintf(" WHERE %s", $whereStatement);
    }
    return sprintf($t_sql, $table, implode(',', $sets), $whereStatement);
}

public static function arrayToWhereStatement($data, $joinType = 'AND') {
    $joiner = sprintf(" %s ", $joinType);
    return implode($joiner, self::arrayToKeyedArray($data));
}

public static function arrayToOrderBy(array $data) {
    $o = [];
    foreach ($data as $field => $direction) {
        $directionBool = substr(strtolower($direction), 0, strlen($direction)) === substr('descending', 0, strlen($direction));
        $o[] = sprintf($field . ' %1$s', ($directionBool ? 'DESC' : 'ASC'));
    }
    return implode($o, ',');
}

public static function arrayToKeyedArray($data) {
    return array_map(function($value) {
        return sprintf('%1$s = :%1$s', $value);
    }, array_keys($data));
}

Answer 1

It's not so much about the state than it is about coupling. When you test a method, you want to control its dependencies; when the method under test calls a static method, you can't control that dependency and thus, the method under test is coupled with the implementation of that static method, so you might as well consider that static method inlined into the method under test, as far as testing goes.

So, you've called this class Utility. Like the Helpers and Managers of this world, a class called Utility is pretty much an incentive to become a dumping ground for just about anything that's, well, a "utility". Name things after what they're responsible for, and thank yourself later: in this case, a SqlStatementBuilderUtility would make it much clearer what the intent is, and then the class would be much less prone to grow some bubbleSort or sendMail "utility" members in the future.

Side note, the indentation is off; it's like the class-level scope doesn't exist, the class and its members are all at the same indentation level, and that doesn't look right.

I don't think static is the main concern here - indeed, these methods look like good candidates for being static. The problem is what they're being used for. It looks like it's building SQL statements out of whatever values are thrown at it, which obfuscates the actual queries your application is sending to the database server, and raises a flag about sql-injection.

Answer 2

Interface

For me, it's quite unclear how I would use this class. My guess is this:

echo Utility::arrayToInsertStatement("mytable", array("foo" => "bar", "foo1" => "bar1"));
echo "<br>";
echo Utility::arrayToSelectStatment("mytable", array("foo" => "bar", "foo1" => "bar1"), "foo = 'bla'");
echo "<br>";
echo Utility::arrayToUpdateStatement("mytable", array("foo" => "bar", "foo1" => "bar1"), "foo = 'bla'");

Which is already not all that readable. It results in this:

INSERT INTO mytable (foo, foo1) VALUES (:foo, :foo1)
SELECT bar,bar1 FROM mytable WHERE foo = 'bla'
UPDATE mytable SET foo = :foo,foo1 = :foo1 WHERE foo = 'bla'

But this is really not intuitive. arrayToInsertStatement uses the array keys, but not the values, while arrayToSelectStatment uses the values, not the keys.

If you go the OOP route, the code could be self documenting. You might have something like SelectStatement which has as methods fromTable, addSelectColumns, etc. Then it might be used like this: selectStatement->fromTable("table")->addSelectColumns("colA", "colB");

You should also write documentation for the function, so users know how to use them without looking at the code itself.

Usability

I don't really see the need for functions like these. They make code less readable and obscure the SQL queries themselves. They are also not very powerful, so you will have a mix of hand-written SQL queries and use of these functions.

If you do not want to write SQL queries by hand, you should look at the interface of existing Query Builders such as laravel or the query builder of doctrine and model your code after them, as they are a lot more flexible and result in more readable code.

Security

If you go to the trouble of writing something like this, I would not give users the possibility to add arbitrary SQL queries via a $whereStatement parameter, which could lead to SQL injection.

A user will have trouble seeing which values are safe and which are not (of course, in this case none of the input variables should be user supplied, as it would lead to SQL injection in all cases. But a user of the function may not know this, and may assume that you validate the input in some way). At the very least this should be commented on.

Misc

• arrayToWhereStatement, arrayToOrderBy, and arrayToKeyedArray should probably be private, as they do not seem to have any use outside of the class.
• you forgot the type hint in arrayToWhereStatement.
• sometimes, sprintf results in more readable code. But eg here: $joiner = sprintf(" %s ", $joinType);, $joiner = " $joinType " seems just as good, and shorter.
• o is not a very expressive variable name.