Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Argh... First off: PDO derived and/or wrapper classes are IMHO completely pointless things. I've been rather vocal about this 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.

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.

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.

added 5726 characters in body
Source Link

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

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

Source Link

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.

default

AltStyle によって変換されたページ (->オリジナル) /