current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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?

share|improve this question
1  
Code like this is why I prefer ORM offerings like Doctrine. –  Steven Jan 25 '11 at 16:24
2  
Very usefull comment. –  Jefffrey Jan 25 '11 at 16:31
    
Where's little Bobby Tables when we need him? –  BenV Jan 25 '11 at 20:51
add comment

2 Answers 2

up vote 4 down vote accepted

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 :)

share|improve this answer
    
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? –  Jefffrey Jan 25 '11 at 18:38
1  
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 Jan 25 '11 at 20:31
    
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 –  edorian Jan 25 '11 at 20:34
    
+1 for "creepy magic". I did a double-take when I saw that as well. –  BenV Jan 25 '11 at 20:49
    
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. –  Jefffrey Jan 26 '11 at 7:24
show 1 more comment
up vote 1 down vote

You definitely want to clean the input a bit more before submitting to the database. This is particularly true for your insert, update, and delete clauses. At the very least, run htmlspecialchars and strip_tags, but you should probably check for more specific requirements as well.

share|improve this answer
2  
You are suggesting to strip out everything that looks like html out of an generic database handler. That seems very odd to me. Without knowing in which context the data is used you can't filter it. A comment containing "Hallo & Goodbye" shoudn't be saved as "Hallo & Goodbye" in the database. If you put it in a PDF (for example) it will be broken. The database handler should use prepared statements or escape the data and when putting out into HTML the code that does the outputting should escape the data. –  edorian Jan 25 '11 at 20:41
    
You have the question tagged "php", "php5", and the code is in php, so I assumed this was a php-specific application. Also, if you're going to be the only one using it, then it makes no difference where the data cleaning goes; here or in app logic. If you're handing this off to others, this is far too weak an abstraction for general purpose use (subqueries, order by, complex joins are not supported without db-specific syntax in the where clause, etc.), hence my assumption. –  eykanal Jan 25 '11 at 21:01
    
I'm not the one that asked that question (just to clear that up) and i still think it very much matters where that happens because at the point the input reaches the database level there is no context information available on how to clean the values put in. It could be a picture you insert into the database. And with binary data stripping out (or changeing) every "&", "<" or ">" out of that binary data is going to ruin it. –  edorian Jan 25 '11 at 22:08
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.