2
\$\begingroup\$

I am moving in to OOP PHP and having some problems extending my database class to my objects. I am still new to the variable scoping in OOP PHP, so if it a tiny syntax error I apologize. If my methodology looks wrong or can be improved, I would love to hear some feedback!

    class Database {
    /** Default Port is */

    private $host = 'localhost';
    private $user = '';
    private $password = '!';
    private $db_name = '';
    private $port = ;


    private $connection;
    static protected $instance;

    protected function __construct($host, $port, $user, $password, $db_name) {
        $this->host;
        $this->port;
        $this->user;
        $this->password;
        $this->db_name;
        $this->connect();
    }

    private function connect() {
        try {
            $this->connection = new PDO("mysql:host=$this->host;port=$this->port;dbname=$this->db_name", $this->user, $this->password);
            $this->connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }
        catch (PDOException $e) {
            echo 'Connection failed: ' . $e->getMessage();
        }
    }

   public static function getInstance() {
       if(!self::$instance) {
           self::$instance = new self($host, $port, $user, $password, $db_name);
       }

       return self::$instance;
   }

   public function __call($method, $args) {
       $callable = array($this->pdo, $method);
       if(is_callable($callable)) {
           return call_user_func_array($callable, $args);
       }
   }
}

class Main {
   protected $db;

   public function __construct() {
      $this->db = Database::getInstance();
   }   
}

class aeroplane extends Main {

    function __construct() {
       parent::__construct();
    }

    public function insert_plane($arg) {

    } 

    public function update_plane($arg) {

    }

    public function delete_plane($arg) {

    }

    public function findById($id) {
        $qry = $this->db->prepare('SELECT * FROM planes WHERE userID = :userID');
        $qry->execute(array(':userID' => $id));
        $results = $qry->fetchAll(PDO::FETCH_ASSOC);

        return $results;
    }   
}
$db = new Database;
$plane = new aeroplane;
echo $plane->findById(1);
\$\endgroup\$
0
3
\$\begingroup\$

Argh... First off: PDO derived and/or wrapper classes are IMHO completely pointless things. I've been rather vocal about this. That being said, the thing that upsets me most of all looking at your code is that you've done what most of us did when we learned about OOP in PHP, and wrote DB connection classes. You've gone and used a singleton.

Singletons are evil for a number of reasons:

  • They're a lot harder to test
  • They're pointless in a stateless environment
  • They're limiting. Using a singleton limits your options sooner than you thing (more on this later)
  • They're globals in OO drag, and globals are generally bad
  • There are better alternatives you can use:

Here's a flowchart you should follow if ever you think you need a singleton:

Singleton flowchart

A quick question concerning the actual validity of your code, though:

public static function getInstance()
{
    if(!self::$instance) {
        self::$instance = new self($host, $port, $user, $password, $db_name);
    }

    return self::$instance;
}

Where are the $host, $port, $user, and other variables coming from here? As it stands, this code can't possibly work. Also: once this singleton of death is used to create one connection, it can't be used a second time to connect to another DB. Having more than one DB connection is really quite common, and indeed very, very useful in real life.
Remember I said that Singletons are rather limiting? Add a second DB to your application, and your singleton becomes an obstacle, a nuisance that everybody hates, and wants to get rid of. Believe me when I say this happens a lot faster than you might think.

I'll give your code a thorough once-over when I get home from work, so stay tuned for updates. For now, I'll leave you with one last piece of advice: please, follow the coding standards. Coding style matters, a lot. Trust me.

Update:
I seem to be rather short of time lately, but still I managed to find a couple of minutes to address another issue with your code. You seem to be confused as to what the constructor does in an inheritance scenarion:

function __construct()
{
   parent::__construct();
}

Clearly, you've found out that, if a child class defines a constructor, the parent constructor is not invoked. The reason for this is simple: The child class defines its own constructor, effectively overriding the one defined by the parent. Hence, the parent constructor is ignored, but can be called using parent::__construct();. That's fine, but if your child constructor doesn't do anything, except for forwarding the constructor call to the parent, why bother overriding the constructor? Just leave the parent be:

class Foo
{
    protected $val = null;
    public function __construct($assign = null)
    {
        $this->val = $assign;
    }
}
class Bar extends Foo
{
    public function getVal()
    {
        return $this->val;
    }
}
$bar = new Bar('A value');
echo 'Bar uses the constructor of Foo, so $val is now: ', $bar->getVal();

The output will be "Bar uses [...] is now: A value". Nice and easy, and more importantly: I'm not repeating myself needlessly. You mention you're quite new to scoping and OO, so I'll just give you the basics:

  • Properties are scoped on a class level: public means they can be accessed anywhere, but the access needs to happen through an instance (non-static) or through a class (static). Inheritance doesn't change this: the child inherits the property from the parent, and it is public
  • protected properties can only be accessed through the class itself, or a child thereof. Like in the snippet above: $val is protected, so both Foo and Bar can access it freely, but $instance->val is not allowed.
  • private properties can only be accessed from within the class itself. Children cannot access something that was defined private on the parent level. They can, however, override it and make it.
  • When properties or methods are overridden by the child, the access modifier (private, protected, public) must either be preserved, but can be made less strict. making it stricter is not allowed (ie parent declares property protected, the child can declare it protected or public, but not private)
  • When overriding methods, the same rules apply (access modifier may not be strengthened), but more rules are added: the signature defined by the parent is part of the inherited contract. The number of arguments a method expects, the expected types and preferable expected return type must be upheld. Additional arguments are allowed, provided they are optional
  • class methods can't access variables outside of the class definition. While it is technically possible to access a global variable using global $someVar; this is considered extremely unsafe, and bad practice. A class should be a self contained unit of code, and not rely on global variables being set to a specific state.
  • Methods have the same scope as functions: local variables exist until the method returns. Properties retain their state for the lifetime of the instance. static properties and variables that can be declared in methods, too, retain their state indefinitely (ie: until the end of the request) - Example below.
  • The keyword to create a child class is extend, not alter. While this might seem obvious, avoid the trap of changing the behaviour of a class when extending it. All you should do is add functionality, never change it. A child should be able to do what its parent did, and then some more.

Example of static state:

class Foo
{
    public $p1 = 0;
    public static $P2 = 0;

    public function test()
    {
        $local = 1;
        static $local2 = 1;
        return array(
            'local'  => $local++,
            'local2' => $local2++,
            'p1'     => $this->p1++,
            'P2'     => self::$P2++
        );
    }
}
$x = new Foo();
print_r($x->test());
print_r($x->test());
unset($x);//unset instance
$f = new Foo();
print_r($f->test());
print_r($f->test());

This code should yield the following output:

Array
(
    [local] => 1
    [local2] => 1
    [p1] => 0
    [P2] => 0
)
Array
(
    [local] => 1
    [local2] => 2
    [p1] => 1
    [P2] => 1
)
Array
(
    [local] => 1
    [local2] => 3
    [p1] => 0
    [P2] => 2
)
Array
(
    [local] => 1
    [local2] => 4
    [p1] => 1
    [P2] => 3
)

As you can see, the local variable will always be 1: it's initialized to 1 when the method is being called, and GC'ed (Garbage collected) when it returns. The static local, though, retains its value inbetween calls, and is therefore always incremented, which results in its value being 1, 2, 3, 4 in each returned array.
The static property behaves in the exact same way as the static variable, apart from it being initialized to 0, instead of 1, which is why its value is 0 through 3, rather than 1 through 4. The non-static property is being incremented after each call on a particular instance, but it does not share its state between various instances.

These are just some general truths and rules of thumb, but they're important. When it comes to scoping, it's really all rather simple, though

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

Ah, this subject has already been discussed here recentely and extensively. See:

Small PHP DAO app

The comments there are useful to you. The basic idea is that the PDO class is quite good as it is. You can wrap it, like you do, extend it, like I suggested, but what's really been added? Not much, in your case.

You have implemented the __call() method, so PDO is accessible from outside your database class. That's good, but all you add is making the connection. That might not be enough to justify the wrapping.

I noticed that you echo when a connection fails. That's not what such a class should do. It should return with an error status. Classes should always have one resposibility, yours has two: creating output and connecting to a database.

You also don't seem to know what you want to do with the result of the database class. I see a $db in the global scope, but also one in the Main class.

There's no real error checking. yes, you output a message when the connection fails, but you cannot, and do not, act on it. There's no status returned from the database class, and there are no checks to see if the connection actually was made before you go on.

\$\endgroup\$

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.