8
\$\begingroup\$

The point of this question

I'm actually using it while developing a simple application and it seems to cover all my needs. Also it uses PDO so that we don't really have to worry about SQL Injection. I know I usually code strange, but I hope you could give me suggestions and feedback in order to improve it.

Code: Database Class

/* Operate on the database using our super-safe PDO system */
class db
{
    /* PDO istance */
    private $db = NULL;
    /* Number of the errors occurred */
    private $errorNO = 0;

    /* Connect to the database, no db? no party */
    public function __construct()
    {
        try
        {
            $this->db = new PDO(
                'mysql:dbname='.reg::get('db-name').';host='.reg::get('db-host'), reg::get('db-username'), reg::get('db-password')
            );
        }
        catch (Exception $e) 
        {
            exit('App shoutdown');
        }
    }

    /* Have you seen any errors recently? */
    public function getErrors() { return ($this->errorNO > 0) ? $this->errorNO : false; }

    /* Perform a full-control query */
    public function smartQuery($array)
    {
        # Managing passed vars
        $sql = $array['sql'];
        $par = (isset($array['par'])) ? $array['par'] : array();
        $ret = (isset($array['ret'])) ? $array['ret'] : 'res';

        # Executing our query
        $obj = $this->db->prepare($sql);
        $result = $obj->execute($par);

            # Error occurred...
            if (!$result) { ++$this->errorNO; }

        # What do you want me to return?
        switch ($ret)
        {
            case 'obj':
            case 'object':
                return $obj;
            break;

            case 'ass':
            case 'assoc':
            case 'fetch-assoc':
                return $obj->fetch(PDO::FETCH_ASSOC);
            break;

            case 'all':
            case 'fetch-all':
                return $obj->fetchAll();
            break;

            case 'res':
            case 'result':
                return $result;
            break;

            default:
                return $result;
            break;
        }
    }

    /* Get PDO istance to use it outside this class */
    public function getPdo() { return $this->db; }

    /* Disconnect from the database */
    public function __destruct() { $this->db = NULL; }
}

Use

$db = new db;
$user = $db->smartQuery(array(
    'sql' => "SELECT UserName FROM `post` WHERE UserUID = :uid",
    'par' => array('uid' => $uid),
    'ret' => 'fetch-assoc'
));
echo $user['Username'];

What I think is wrong

Well, I have encountered these 2 points while revisiting this code, and I'd like to get some feedback about them particularly:

  1. The error system (which, let's face it, sucks right now)
  2. The first try-catch code, which is actually working, but I never use that so, please, look at it.

Also if my application cannot connect to the database, most (if not all) features cannot be activated (such-as the PHP error log trough a database record, so that every times an error occurred, the admin is warned trough the application itself).

\$\endgroup\$
  • 1
    \$\begingroup\$ What is PDO? I've never heard of it before. \$\endgroup\$ – John Kraft Feb 4 '11 at 17:18
  • 6
    \$\begingroup\$ Google it? And anyway are you kidding me? \$\endgroup\$ – Shoe Feb 4 '11 at 18:26
  • 1
    \$\begingroup\$ Nope. Wasn't kidding you. I have limited experience with PHP, and it's been several years since I have touched it; all pre-PHP5. You prefaced your question with "I know I usually code strange", so I thought it was something you made up. \$\endgroup\$ – John Kraft Feb 4 '11 at 20:07
  • 1
    \$\begingroup\$ No, oh. Well PDO stands for PHP Data Objects (PDO) which is a db extension to interface better with a database. PDO is also safer than anything(?) because it uses Prepared Statements preventing SQL Injection that could be damage your database. For more information please visit the PHP documentation regarding it: php.net/manual/en/intro.pdo.php \$\endgroup\$ – Shoe Feb 4 '11 at 20:55
  • \$\begingroup\$ maybe you could check out this project, quite a clear and good design: github.com/poplax/PHP-light-PDO-Class \$\endgroup\$ – zx1986 Nov 6 '12 at 8:30
5
\$\begingroup\$

A few things.

When you connect, don't hard code mysql, you are already fetching all the db info from a registry/config ... make the the dbtype configurable as well. That is after all, the point of PDO.

When you catch errors in connection or when executing a query, use the PDOException class

use exceptions for your prepare and execute calls instead of counting the errors that happens...as that's not really useful (it will be obvious an error happened, what I care about is what that error actually is).

personally, I would break down this class into specific methods. instead of connecting to the db in the constructor, I would have a connect() method to do this. I would also have a query, prepare, and bind method and then have interaction method: delete, update, insert and then a set of fetch methods, fetchOne, fetchAll, fetchCol, etc.

Also the connect method would check for a set conn, and only attempt to connect if you didn't already have an existing connection. The query method would call connect(). The reason for this is that you may include your db class throughout your code, even if you dont fetch anything in particular requests...with your code if this is the case a connection will be made regardless. What you want is lazy loading...meaning a db connection will only be made if you actually attempt to use the db with a fetch/update/delete/insert/etc.

Here is some sample pseudo code

$db = new MyPdoClass();
$db->connect();
$stmt = $db->query($sql, $params);
$results = $stmt->fetchAll(PDO::FETCH_ASSOC);
// or call your MyPdoClass::fetchAll, which would do that so you would just call
$results = $db->fetchAll($sql, $params);
\$\endgroup\$
  • 1
    \$\begingroup\$ Are there samples of your class type? \$\endgroup\$ – Shoe Feb 7 '11 at 12:53
3
\$\begingroup\$

Why not make DB a Singleton class?

Making it Singleton will prevent multiple sign-ons. Now in your code, each time a DB object is created, you authenticate with the database server.

If you make it Singleton, you just have to connect once. And whenever you need the instance, you just have to fetch it.

\$\endgroup\$
  • 1
    \$\begingroup\$ Singletons are generally considered big no no's since they're essentially global stateless objects (and we all know how evil globals are) which makes them untestable. \$\endgroup\$ – xzyfer Feb 10 '11 at 12:21
  • \$\begingroup\$ Well, @xzyfer, I'd like to point you to stackoverflow.com/questions/11831/… - a view at the first answer will give you why Singleton can be used here. \$\endgroup\$ – mauris Feb 10 '11 at 15:15
  • 1
    \$\begingroup\$ "Why not make DB a Singleton class?" Because the singleton pattern is an anti-pattern, makes testing a lot harder than it needs to be, it's pointless in a stateless environment (which PHP is), and it's just generally evil there's no valid reason to use them in PHP \$\endgroup\$ – Elias Van Ootegem Dec 4 '14 at 15:29
1
\$\begingroup\$

Coupling

The first thing that comes to mind is the tight coupling between db and reg.

Since db doesn't actually need to know anything about reg, and only needs a few values from it, I feel it's better practice to pass the required values in as needed:

public function __construct($host, $database, $username, $password)
{
    ...
}

Exception handling

As mentioned in another answer, catch PDOException instead. Also, you will have a lot of frustration down the track trying to figure out what an "App shoutdown" error means with improved error reporting.

try
{
    $db = new DB(...);
}
catch (PDOException $e)
{
    logError($e);
    exit('Sorry, the site is broken!');
}

Commenting

phpDoc comments can prove invaluable as the size and complexity of classes grow. Even if not strictly necessary for simple projects, I've found it a useful habit to get into.

\$\endgroup\$
  • \$\begingroup\$ #Coupling : Well, actually reg has now been replaced with con and stands for configuration, having thousands of different values. Also not setting any parameters in the construct give me the possibility to get the db class just with $db = new db;, and you will notice that is really fast. #Exception handling, #Commenting : Nice tips. \$\endgroup\$ – Shoe Feb 7 '11 at 17:06
  • \$\begingroup\$ @Charlie, more reason to stay away from that sort of thing. Imagine now that you had reg in 1500 files. \$\endgroup\$ – Kevin Peno Mar 8 '11 at 0:24
1
\$\begingroup\$

I'd avoid Singletons - you can limit access by using a dependency injector like YADIF, which makes writing tests against components that rely on the connection easier to create (as well as testing the class itself)

This is a tricky question - DBAL (Database Abstraction Layers) is a deceptively short acronym for a very broad (and hotly debated) topic.

If you want to keep it light, you can write a Transaction script:

http://martinfowler.com/eaaCatalog/transactionScript.html

Basically an OOP wrapper for your apps most common queries (i.e. saveCompanyEmployees, deleteEmployee, selectEmployee etc.).

You can take this up a notch and use assemble queries programmatically, in which case your methods may become more like this:select($table,$id), selectWhere($table,$condition) - this is more of a Data Access Object than a Transaction Script

If you find yourself moving towards a fluent interface, or class representations of tables - then you may well want to consider an ORM of some description (it's not really worth writing your own, although it has the advantage of being more tailored to your app and possibly more lightweight, and of course you learn the hard way - which is often the best way...), although these can be a mixed blessing - and personally I think it's slightly perverse to be so averse to SQL to hide it entirely in this manner.

I'd reserve your direct SQL execution as a "back door" method you can resort to when all else fails - since having to write the SQL in your application logic kinda ruins the encapsulation benefits of OOP

\$\endgroup\$
0
\$\begingroup\$

I'm assuming you're using PHP5, in which case do you have a good reason not to use existing libraries like Doctrine2 DBAL the defacto standard for database abstraction, or Pear_MDB2?

In this day and age of open source libraries, rolling your own home made (and possible error prone class) to solve already thoroughly solved problems is doing yourself and the PHP community at large more harm than good don't you think?

These libraries are well documented (not so much in the case of Pear_MDB2), thoroughly tested and have 1000s of hours of production use in a myriad of environments. And shoudl you find an issue or have a suggestion you'd be helping the PHP community. At the very least have a look at these fantastic projects, you might learn something and possible even answer some of your own questions should you insist on doing your own way.

Admittedly Pear_MDB2 is a bit behind the times now, but it does a solid job

\$\endgroup\$
  • 1
    \$\begingroup\$ I love to create my own things. It's a way to learn by your own. I usually hate everything which is premade by someone else and I don't really think it worth something to learn an entire framework while you are developing such a simple web application as I do. \$\endgroup\$ – Shoe Feb 10 '11 at 19:34
  • 1
    \$\begingroup\$ Doctrine2 is potentially quite a heavyweight response that comes with the same burden when covering very wide range of use cases. There are a whole range of options available within the term DBAL \$\endgroup\$ – sunwukung Feb 20 '11 at 22:57
  • \$\begingroup\$ For the need described he could easily get away with just the DBAL component which is very light weight. Since Doctrine2 is set of loosely coupled components, the ORM layer wouldn't be necessary in this case. \$\endgroup\$ – xzyfer Feb 21 '11 at 4:03

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.