Am I using OOP correctly?

Question

I've been developing a PBBG (game) written in PHP & MySQL for 7 months. The project is almost done and might be released soon, but I am worried about some issues.

This was my first time programming a real project, and also my first time using OOP. My code now has about 25k lines, and here is my question:

Am I using OOP correctly? I believe I'm calling classes constructs excessively. I have about 20 different classes, and sometimes they need each other. Here is a sample code how I use OOP: (Just a sketch, might not work)

class A {
    
    public $b;
    public $c;

    public function __construct(){
        $b = new B();
        $c = new C();
    }

    public function sampleA(){
        //some stuff that depends $c->sampleC
    }

}

class B { 

    public $c;

    public function __construct(){
        $c = new C();
    }

    public function sampleB($id){
        return $this->c->sampleC($id);
    }

}

class C {

    public function sampleC(){

    }

}

Then I'd use:

$a = new A();
$b = new B();
$c = new C();

$whatIWant = $a->sampleA($b->sampleB($c->sampleC()));

While I could just

$a = new A();
$whatIWant = $a->sampleA($a->b->sampleB($a->c->sampleC()));

This looks uncalled-for on this simple example, but I'm having on my script more than 200 different constructor calls, I guess this will slow down and overload my server.

Maybe my example wasnt very clear, so I took a print of my xdebug profiler result:

Is singleton what I need? I tried using a simple example like this with singleton, but I had the same result (more than 1 construct per class).
Maybe I need to extend these classes and use parent's constructor? But how will it be possible with 20 different classes, sometimes independent, and sometimes dependent of each other?
Or maybe I am using it the correct way (what I dont think so)

A singleton is nothing more than an [hackish] approach to use a stable identifier to refer to a particular instance. Perhaps the words "dependency injection" warrant exploration? — , Sep 03 '12 at 19:21
with 25k LOC and only 20 classes you get 1250 LOC per class. Even if half of that is NCLOCs it means your classes are way too big and you are likely not following SRP. — Gordon, Sep 03 '12 at 20:44

score 3 · Accepted Answer · edited May 23 '17 at 12:09

I wouldn't do it like that. The way I would do it is by using dependency injection. I've set the instances in the classes to protected (or set them to private). There is no need to set them public. If you think you need that you are almost certainly violating some rules and you would need to rethink your design.

Another thing I've changed is that you did $b = new B(). There are two problems with this:

I think you meant to do $this->b = new B()
You are tightly coupling the B class to the other class.

This makes it impossible to do any unit testing, because you cannot replace the B class with a mock class. So instead of testing the unit (the class) you are testing mutiple classes.

Another thing I have done is type hinted the constructor parameters. This would be even more useful when you have interface for the classes. This also makes it so much easier to test the classes. Also see this related answer.

Some videos you may be interested in: Dependency Injection, Unit testing, Inheritance, Polymorphism, & Testing. You may also want to read a bit about SOLID programming. You code also seems to violate LoD meaning you are trying to access stuff through other objects.

Something like this is what I would have done:

class A
{
    protected $b;
    protected $c;

    public function __construct(B $b, C $c)
    {
        $this->b = $b;
        $this->c = $c;
    }

    public function sampleA()
    {
        $this->c->sampleC();
    }
}

class B
{     
    protected $c;

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

    public function sampleB($id)
    {
        return $this->c->sampleC($id);
    }
}

class C
{    
    public function sampleC() { }
}

$b = new B();
$c = new C($b);
$a = new A($b, $c);

$whatIWant = $a->sampleA();

If you want to write correct OOP code you shouldn't ever (imho) make use of the signleton anti-pattern. Basically it is just another name (with the same characteristics) as a global.

Maybe I need to extend these classes and use parent's constructor? But how will it be possible with 20 different classes, sometimes independent, and sometimes dependent of each other?

You should only extend classes when the child class can substitute the parent class. This is called the Liskov substitution principle in SOLID. In other words you have to be able to say class child is a class parent.

score 2 · Answer 2 · answered Sep 03 '12 at 19:21

What you need is the Inversion of Control pattern in conjunction with Dependency Injection.

With those patterns you can keep your classes losely coupled, and enforcing that every class is constructed with the correct classes and parameters.

Most good PHP frameworks have a IoC and DI implementation. Look at this tutorial from Symphony.

Am I using OOP correctly?

2 Answers2