Database class in PHP; only has one function - question about practices and whether it's a good idea

Question

So I have this database class in PHP and I only have 1 function in it (other than __construct and __destruct. Let me explain further...

I had originally written it so when I connected to a database I would just call my function connect_to_db() which returned a mysqli object. I then used this objects functions (->query(), ->prepare(), ->bind_param(), et cetera..) to do whatever. This cluttered (and still is as I haven't switched my code over to my new class yet) with a lot of functions that just do specific things, for example:

function get_country_info($db, $usrid) {
  $statement = "select `name`, `population`, `money`, `food` from `countries` where `usr_id` = ?";
  $qry = $db->prepare($statement) or
    trigger_error("get_country_info():statement failed...");
  $qry->bind_param("i", $usrid);
  $qry->execute();
  $qry->bind_result($name, $population, $money, $food);
  $qry->fetch();
  $res = array("name" => $name, 
    "pop" => $population, 
    "money" => $money, 
    "food" => $food);

  return $res;
}

And what I originally planned to do was just to throw these into a class for clarity, but what I actually ended up doing was creating a class that when created creates a mysqli object for a specific database (supplied via an argument) and when destroyed closes off this link. I very much like not having to worry about typing $db->close(); as when the script finishes executing the connection is closed.

It only has 1 other function; query(). This function can handle any query put forth and outputs it as a multidimensional array. It uses eval(). From what I understand this is generally frowned upon - but eval() is so useful; so why is it frowned upon?

I guess the function may be slow, but seen as I'm just using it for a pet project (browser based country management game) and I haven't noticed anything I'm not worried about that at all. What I am worried about is whether people would generally do this in the 'real world' or whether there is some sort of standard by which people do this, or perhaps something included (or add-on-able) in PHP which is usually used?

I'm self taught, so sometimes I don't really know the best way to go about doing things. Perhaps somebody could advise me here?

The function in question can be seen here: http://pastebin.org/353721, as its ~60 lines and I don't want to clutter the page. I also haven't extensively tested it (I don't even know what unit tests are, so I test by using the function in a lot of different ways) so it may have problems I'm not aware of that you guys can point out.

Thanks.

Answer 1

Doesn't seem to me too bad. You have basically done a really basic wrapper around the database, and you performed the very first step in database independence, which is good.

This can be good for small and well organized projects, but once you will start going more complicated, you will soon notice that your approach has one large disadvantage: you still have your SQL queries split around the site. The day you will change anything in the database, you'll need to go through all your site and look for all the SQL statements: this is bad (I had to do it once...).

You should now move all the SQL code to one place. So there are various options. In a similar context, I made the query() method protected, then made the Db class "abstract" and subclassed it. Each subclass is a class which contains several methods relative to a specific set of tables, and this is the only place where you find SQL. All the rest of the project can only call these methods.

Or, even better, you could use an ORM (oblect relational mapper) which would map each table to an object.

For what concerns the evils of evals: I never used them. But I thought I wasn't going to use a "goto" because it's evil, and then I found the magic place where that was the perfect fit. So, if you have investigated all the possibilities, and you think that's the optimal solution... use it.

Answer 2

First: The eval() in your function doesn't serve any purpose as I see it. Why don't you call the $qry->... things directly?

Second, your function is way to complex. Brake it down into smaller parts that call each other.

Third: Manipulating SQL Statements is generally a very bad idea. It's error prone as soon as you feed your function with something like nested statements or other more complicated SQL code. So just don't do it.

Fourth: I wouldn't recommend using mysqli anymore. You should switch to PDO.

As I understand it, your function tries to find out, what fields were in the SELECT clause of the query to build the result array. That is only necessary if you want to allow your database layer it to execute arbitrary queries which is also not a good idea.

When I have to use classic SQL in an application, I predefine every query that I need in some method or function. That function then will know exactly what field it has to read from the result set. So there's no need to parse the SQL code. However, mostly I try to avoid SQL by using things like Doctrine ORM. SQL is just too prone to errors...

About eval(): It is entirely and absolutely evil for several reasons:

• It may open the door to code injection.
• It makes code unreadable to humans and unparsable to an IDE
• It's nearly impossible to debug code that uses eval()
• Not really a problem of eval() itself, but when you have to use it, that's a sign of a flawed design.

Answer 3

• Instead of eval, you can use call_user_func() or call_user_func_array().
• You should check the return value of execute().
• Your function does not support subselects which contain multiple comma's, but only return one value.
• You can use trim() instead of ltrim(rtrim()).

I would still use a function like get_country_info() on top of this db class. Often, you want to do something with the data before your application can use it. More importantly, you want to abstract the data storage from your application. That is, the method which uses the country info does not need to know that it came from the database.

Answer 4

To your question - you need some experience to learn why there are "best practices", why eval() is deprecated, why people use prepared statements etc.

I'd recommend you to have a look at http://dibiphp.com/cs/ for a nice and concise DB layer for PHP.

You could also try Java when you get some clue about web technologies. Then you can use cool things like iBatis, which almost got it to PHP

I personally once had a lib for DB, which was used like this:

$aasAreas = Array();
$sSQL = "SELECT id, x, y, x2 AS r, popis FROM ".$oFW->GetOption('tables.areas')
         ." WHERE id_ad=".asq((int)$_GET['id']);
$oRes = $oFW->GetDB()->Select($sSQL);
if(!$oRes || !$oRes->IsOk()){ $sError = $oRes->GetError(); break; }
else while( $a = $oRes->FetchRow() ){
   $aasAreas[] = $a;
}

[OT] And some ORM for PHP attempt, used like this:

// Load by ID
echo "<h3>Load by ID</h3>";
$iID = 1;
echo "<pre>\$oObject = \$oOP->LoadObjectById(".$oClass->GetName().", $iID);</pre>";
$oUser = $oOP->LoadObjectById($oClass, $iID);
echo "<pre>oUser: [".gettype($oUser)."]".AdjustedPrintR($oUser)."</pre>";
echo "<div>GetPoolCount(): ".$oOP->GetPoolCount()."</div>";

// Save
echo "<h3>Save</h3>";
$oUser = new cObjectPersistenceTestClass_User();
$oUser->SetId(1);
$oUser->SetProperty('user', 'as'.rand());
$oUser->SetProperty('pass', 'as');
$oUser->SetProperty('fname', 'Astar');
$oUser->SetProperty('lname', 'Seran');
echo "<pre>oUser: [".gettype($oUser)."]".AdjustedPrintR($oUser)."</pre>";
$bSucc = $oOP->SaveObject($oUser);
echo "<div>".($bSucc ? 'saved' : 'error')."</div>";

// Load by value - load object created above -> Object Pool hit
$xVal = $oUser->GetProperty('user');
$aoUsers = $oOP->LoadObjectsByValue($oClass, 'user', $xVal);
echo "<pre>\$aoUsers: [".gettype($aoUsers)."]".AdjustedPrintR($aoUsers)."</pre>";

etc.

Answer 5

As Techpriester wrote: brake it down into smaller parts. I recommend at least:

• the argument to bind converter
• the single and multi param bind if-else branch
• the sql statement parser (find out about returned fields)

One other approach to your problem would be to build the sql statement inside your query class (arguments would be field names, the table, the where constraints,... ) and to develop subclasses for different query types. This would elimiate the need to parse the statement. Above this layer you would put a bunch of DAO classes to do the actual work.

But wait: others have done this... (maybe a lot of others to be specific). So using PDO, Doctrine, Propel or even a bigger library like ZendFramework (active record pattern there...) would be the most valuable and stable option.

If you, however, want to learn something about software architecture and OO Systems. Go ahead and build your own, if you do it well you can switch to another DB Layer as soon as your system grows (Version 2.x :-) ).