2
\$\begingroup\$

Is my following (very basic) class considered good use for PDO integration? Or is there a better, more efficient way?

class Page {

    public $title;
    public $db;

    public function __construct($title) {
        $this->title = $title;
    }

    public function connect($host, $dbname, $username, $password) {
        $this->db = new PDO('mysql:host='.$host.';dbname='.$dbname, $username, $password);
    }

    public function disconnect() {
        $this->db = null;
    }

}
\$\endgroup\$
4
\$\begingroup\$

Interestingly, my review is going to be all about control.

Not Enough - Public Property

Just like in the real world, public property can be used by anyone. If your car was public property anyone could walk into your driveway and do whatever they liked with it. When you woke up you could not expect to $this->car->drive() (What if someone had already taken it and replaced it with null?). If they wanted to they could place a house brick in your driveway pretending that it is your car (good luck driving that).

Public properties remove the encapsulation of state from an object. I see virtually no place for them in OO. Protected and Private properties on the other hand are set through using the public interface (the public methods) of an object. Importantly the public interface is testable, so you can ensure that you will not end up with a brick in your driveway unless you really want that.

Too Much - Inversion of Control

Your code will benefit from an Inversion of Conrtol (IoC). You can read about IoC on the web. There is a good answer on stackoverflow with code here.

With IoC and the protected properties your code becomes:

class Page {

    protected $title;
    protected $db;

    public function __construct($db, $title) {
        $this->db    = $db;
        $this->title = $title;
    }
}

Your code is no longer dependent on the specific database setup to function correctly. Your page will use any database connection that it is given. This has big benefits when it comes to unit testing. See Miško Hevery - How to Think About the “new” Operator with Respect to Unit Testing.

I have another answer that covers similar things with an extra part on injecting interfaces here.

\$\endgroup\$
  • 2
    \$\begingroup\$ +1 inversion of control as applied here is also referred to as dependency injection. \$\endgroup\$ – Jack Oct 16 '12 at 12:19
1
\$\begingroup\$

Unused properties

You shoult not set $this->title in the __construct() method unless you are planning on using it.It would be better if you set this value in the particular method that is planning on using this data.

Visibility

The first thing I noticed when I looked at this code is that these properties public $title; public $db;are indeed public. It would be best if you change the visibility to private to ensure that you have more control over the state of these values.

Exception handling

In your connect method you are creating a new PDO object without handling any errors that may occur. The correct approach is to use the PDOException class to handle this error.

An example:

try
{
  $this->db = new PDO('mysql:host='.$host.';dbname='.$dbname, $username, $password);
}
catch (PDOException $e)
{
  die($e->getMessage());
}

Commenting

It's always good to practice making comments in your code to ensure that its easy to follow and if another developer should use your class they will be able to understand exactly whats taking place. phpDoc is a good tool that can be used to generate documentation.

Querying

Is this the complete class? I ask this because it seems as if a query function is missing. Making queries with PDO have been simplified thus can implement a simple query method to retrieve to access your database.

public function query($query)
{
  if($query)
  {
    $current_query->$this->db->prepare($query);
    $current_query->execute();
    $result->$current_query->fetchAll();

    return $result;
  }
  else
  return false;
}

I hope my answers helps somewhat if they are any concerns or errors one can just leave a comment, I am open to criticism.

\$\endgroup\$
  • \$\begingroup\$ Thanks, that all makes sense! I left exception handling out, because I'm just starting to learn OO. And I left out querying because I thought it was redundant, because PDO already has native methods to do that. That's why I also made a public $db \$\endgroup\$ – fishbaitfood Oct 14 '12 at 9:37
  • \$\begingroup\$ ok great! glad I could help. \$\endgroup\$ – Joel Dean Oct 14 '12 at 21:17
  • \$\begingroup\$ @fishbaitfood: Would like to clarify a few things. Doccomments are good, inline comments are not. They clutter your code. Most IDEs know how to collapse doccomments all at once so they are out of the way. As for title, I agree that shouldn't be the sole item in the constructor, but following Paul's example should fix this. And finally, as you have already mentioned, adding the query functionality would be redundant, as would creating a class just to instantiate a database. If you are planning on doing more this is fine, but as is this is redundant. \$\endgroup\$ – mseancole Oct 17 '12 at 13:17

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.