Database method to query

Question

I have a database class that in __construct() initialize a PDO connection and insert the instance into a $db private var. Now i'm working on a method that can be used to query in this way:

$db = new db;
$db->query(array(
 'select' => 1,
 'from' => 'table',
 'where' => array('id' => 1, 'name' => 'charlie'),
 'limit' => array(1, 5)
));

I did something that works pretty nicely long time ago while PDO was something unknown, but i was wondering:

1. How could i improve this code a bit
2. How can i end it? I mean how to use the PDO then to submit the query?

Here's the method query():

# Defining the type
 if (isset($array['select'])) { $type = 'SELECT'; $type_value = (is_int($array['select'])) ? '*' : $array['select']; }
 if (isset($array['update'])) { $type = 'UPDATE'; $type_value = $array['update']; }
 if (isset($array['delete'])) { $type = 'DELETE FROM'; $type_value = $array['delete']; }
 if (isset($array['insert'])) { $type = 'INSERT INTO'; $type_value = $array['insert']; }
 if (!isset($type)) { trigger_error("Database, 'type' not selected."); } // Error
 # From
 if (isset($array['from'])) 
 {
 $from = 'FROM'; 
 $from_value = mysql_real_escape_string($array['from']); // table cannot be pdoed
 }
 # Where
 if (isset($array['where'])) 
 { 
 if (!is_array($array['where'])) { trigger_error("Database, 'where' key must be array."); }
 $where = 'WHERE'; $where_value = $array['where']; 
 # Fixing the AND problem
 if (count($array['where']) > 1)
 {
 $list = $where_value;
 foreach ($list as $a => $b) { $w[] = "{$a} = {$b}"; }
 $and = implode(' AND ', $w);
 $where_value = $and;
 }
 }
 # Limit
 if (isset($array['limit'])) 
 {
 if (!is_array($array['limit'])) { trigger_error("Database, 'limit' key must be array."); }
 if (count($array['limit']) != 2) { trigger_error("Database, 'limit' array must be two-keys-long"); }
 $limit_first = $array['limit'][0];
 $limit_second = $array['limit'][1];
 $limit = 'LIMIT';
 $limit_value = "{$limit_first}, {$limit_second}";
 }
 # Set
 if (isset($array['set']))
 {
 if (!is_array($array['set'])) { trigger_error("Database, 'set' key must be array."); }
 $edits = $array['set'];
 foreach ($edits as $a => $b) { $e[] = "{$a} = {$b}"; }
 $set = 'SET';
 $set_value = implode(',', $e);
 }
 $vals = array('from', 'from_value', 'set', 'set_value', 'where', 'where_value');
 foreach ($vals as $v) { if (empty($$v)) { $$v = ''; } }
 $sql = "
 {$type} {$type_value}
 {$from} {$from_value}
 {$set} {$set_value}
 {$where} {$where_value}
 ";
# Here there would be something like mysql_query($sql), but i'd like PDO! PDO get me hornier.

And now? How to bind parameters? Is that possible to work it out?

• Steven: Code like this is why I prefer ORM offerings like Doctrine.
• sh03: Very usefull comment.
• BenV: Where's little Bobby Tables when we need him?

Answer 1

That query method (and the db class) has a lot of responsibilities:

• Do the PDO stuff, connection handling
• Be a query builder
• be a query executor
• Handle the params and possibly execute the same statement with different params (it would to that too)
• Do all the error checking
• maybe more i don't see

Usually that functionality is handled in 2 to 3 classes, sometimes even more and not in one single function.

And you are doing some very creepy magic to achieve all the work

foreach ($vals as $v) { if (empty($$v)) { $$v = ''; } }

and all that so you can write

array("select " => "something" , ...

instead of

"select something" ...

Also you are using mysql_real_escape_string so it seems you don't want to use the pdo escaping (not binding parameters) so why try to use PDO if you limit yourself to mysql anyways ?

So far everything i spotted thinking about it for 5 minutes. Will improve upon feedback / direction from you if it helped at all :)

• sh03: I use mysql_real_escape_string only for the table since you cannot use binding parameter for tables. But, wait. You said you would use 2 or 3 classes? Why should you?
• edorian: Because packing all that functionality into one class make one darn big and complicated (read: hart do maintain, test, debug, understand, extend) class. In short: The "once class one purpose" principle. Many "good oop" books and advocates suggest that you are able to describe the purpose of a class in one sentence without saying "and" so you get maintainable (etc. pp. can go into detail if you want) class.
• edorian: You shoudn't use mysql_real_escape_string for table names though, if you use another database that might be exploitable (using a database that uses different commetents than mysql bad things COULD happen. Use a whitelist for the tablename if you want so make it save. Checking that it matches (for example) /[A-Za-z0-9_]/ will be more secure and less (maybe non) exploitable whereas mysql_real_escape_string doesn't to the right thing for this context
• sh03: How could a different class for query building and query execution be any better? Using 3 classes will make you mad managing parent and inherit issues. Also it seems to me thousands of lines of code for nothings. My db class manage all the db class: one class one purpose. But I'm probably misunderstanding that.
• sunwukung: @Charlie - a: 3 classes is far from an inheritance nightmare. b: In this instance, the classes would not be using inheritance, they simply have separate concerns, c: it's good to divide classes when they approach unmanageable levels of complexity, or have too much responsibility: misko.hevery.com/code-reviewers-guide/flaw-class-does-too-much