5
\$\begingroup\$

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));
}
\$\endgroup\$
2
\$\begingroup\$

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 .

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your reply. I agree about coupling. If I do this as a class it can be extended and it can be switched out if necessary. You couldn't be more right about the name. I will update accordingly. As for the injection the primary focus for these functions is to create a statement that can be prepared and checked by PDO. \$\endgroup\$ – David Dec 2 '15 at 22:20
2
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ First let me thank you for your time and review. You invested much time and it should be noted it is appreciated. These functions are used to create simple prepared statements, for use with PDO. I agree they are cryptic and are at a version 0.7. My initial question was around structure. Should this be in a class with all statics or should I do a full blown class. I am taking your vote as a full class. \$\endgroup\$ – David Dec 2 '15 at 22:18

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.