9
\$\begingroup\$

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?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Code like this is why I prefer ORM offerings like Doctrine. \$\endgroup\$
    – Steven
    Commented Jan 25, 2011 at 16:24
  • 2
    \$\begingroup\$ Very usefull comment. \$\endgroup\$
    – Shoe
    Commented Jan 25, 2011 at 16:31
  • \$\begingroup\$ Where's little Bobby Tables when we need him? \$\endgroup\$
    – BenV
    Commented Jan 25, 2011 at 20:51

1 Answer 1

6
\$\begingroup\$

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

\$\endgroup\$
5
  • \$\begingroup\$ 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? \$\endgroup\$
    – Shoe
    Commented Jan 25, 2011 at 18:38
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – edorian
    Commented Jan 25, 2011 at 20:31
  • \$\begingroup\$ 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 \$\endgroup\$
    – edorian
    Commented Jan 25, 2011 at 20:34
  • \$\begingroup\$ 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. \$\endgroup\$
    – Shoe
    Commented Jan 26, 2011 at 7:24
  • \$\begingroup\$ @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 \$\endgroup\$
    – sunwukung
    Commented Feb 20, 2011 at 23:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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