Dependency injection in PHP - simple example review

Question

I am new to OOP and I have recently asked question, which have thaught me that:

every "operation" should be implemented in seperate class,
issue I have faced should be resolved with dependency injection,
I must not have one object for entire application.

Let's say I have the following code:

class FtpConnect {
     public $server;

     private function connect () { ... }

     public __construct ($db, $mailer, $server) {
         $this->server = $server;
         $this->connect();
     } 
}

class Database {
    public $database_property;
    ...
}

class Phpmailer {
    public $phpmailer_property;
    ...
}

$db = new Database();
$mail = new Phpmailer();

$ftp = new Ftpconnect ($db, $mail, "ftp.mozilla.org");

$ftp->db->database_property;
$ftp->mail->phpmailer_property;

Is this a proper approach? It seems that I still have one object $ftp.

Reading property or calling methods like $ftp->db->database_property; is this the proper way?

This may be Not Constructive (`... this question will likely solicit debate, arguments, polling, or extended discussion`) and it may also be better for [Programmers.SE](http://programmers.stackexchange.com/), but I'm not sure about either. And it looks like you might do well to learn/remember the [Law of Demeter](http://en.wikipedia.org/wiki/Law_of_Demeter) - FTP probably shouldn't know anything about Mail or DB — DaveRandom, Oct 15 '12 at 12:29
@DaveRandom When FTP connection is established, I want to save datetime, hostname to database and send it with email. That's why classes are dependent, I think. — user1292810, Oct 15 '12 at 12:39

markus · Accepted Answer · 2012-10-15T12:47:39.920

There are a few good things and a few bad things in your example! First, congratulations for your serious attempt to learn good programing practices. You're going the right direction!

Good things:

you're using proper constructor injection (form of DI) and therefore you are only writing new statements on the highest level of your example and none inside of classes.
you have written a class for each of the different services, great.

Bad things:

you're accessing properties of db and mail through the ftp instance, don't do that!
you're using public properties, make properties private and use public getters and setters
you're calling main methods in in the constructor, don't do that, you should not call it inside the class but outside.
you're not doing anything with the $mailer instance passed to the ftp instance, does the ftp class really need the mailer class at all?

Improved example, assuming your ftp class really needs both database and mail:

class FtpConnect {

     private $db;
     private $mailer;

     private $server;

     public function connect () { ... }

     public __construct ($db, $mailer, $server) {
         $this->setServer($server);
         $this->setMailer($mailer);
         $this->setDb($db);
     }

     public setServer($server)
     {
         $this->server = $server;
     }

     public setDb($db)
     {
         $this->db = $db;
     }

     public setMailer($mailer)
     {
         $this->mailer = $mailer;
     }
}

class Database {
    private $databaseProperty;
    public getDatabaseProperty()
    {
        return $this->databaseProperty;
    }
}

class Phpmailer {
    private $phpmailerProperty;
    public ... see above...
}

$db = new Database();
$mail = new Phpmailer();

$ftp = new FtpConnect ($db, $mail, "ftp.mozilla.org");
$ftp->connect();

$someProperty = $db->getDatabaseProperty();

Awesome answer! But what's wrong with calling main methods inside the constructor? — , Oct 15 '12 at 12:52
You're binding yourself to a certain way of using the class. Maybe one time you want to make the connection immediately, maybe another time you want to wait until you really need it (lazy connection). Don't define HOW you're gonna use your instance in the constructor! — markus, Oct 15 '12 at 12:56
@markus-tharkun I much appreciate your answer, everything seems clear now. — user1292810, Oct 15 '12 at 13:12
@markus-tharkun I have one more question: do I need all these `set` methods, which are called in constructor? Why not just simply set properties in constructor with `$this->server = $server;`? — user1292810, Oct 15 '12 at 20:29
There are pros and cons for both. If you want to extend the class later, you may want to keep the setters in the constructor. — markus, Oct 15 '12 at 20:46

score 0 · Answer 2 · answered Oct 15 '12 at 12:28

0

Your constructor silently discards everything except the third argument passed to it. Why would you put a db object in ftp? That looks a bit weird.

Other then that, you have already initialized variables for the db/mail objects so you should use them (less chaining makes the code easier to read) or write something like this: $ftp = new Ftpconnect (new Database, new Phpmailer, "ftp.mozilla.org"); Do not make the code more obscure then it has to be.

Do not give the ftp object that what it won't be using. Also, if the properties you're trying to access won't be instance specific (it will be the same in all the objects of that class) you should use class constants or static variables

answered Oct 15 '12 at 12:28

karka91

733
1
6
20

I disagree with `$ftp = new Ftpconnect (new Database, new Phpmailer, "ftp.mozilla.org");` 99% of the time, it directly contradicts your own statement "Do not make the code more obscure then it has to be" IMHO. – DaveRandom Oct 15 '12 at 12:34
`Why would you put a db object in ftp? That looks a bit weird.` All I want to do is log every ftp_connection to database. When script run and ftp_connection is established I want to save datetime, host name, etc. – user1292810 Oct 15 '12 at 12:35
@user1292810 I would create an intermediate object for that probably, something like `FTPLogger` which contains the database object and is injected into the FTP connector. That way if you change the way your logging is done (e.g. log to file instead) you don't have to modify your FTP connector. See [this](http://en.wikipedia.org/wiki/Law_of_Demeter) – DaveRandom Oct 15 '12 at 12:39
@DaveRandom you are suggesting to pollute the context with unused variables. You'll make the gc's job a hell more easier if you do not make a variable for every tiny bit that won't even be used in that context. user1292810 that makes perfect sense :) – karka91 Oct 15 '12 at 12:39
@karka91 ...but all you are looking at above is some simple class definitions and the bootstrap. Those extra variables make no functional difference in the long run, but they do make the bootstrap easier to read. IMHO. – DaveRandom Oct 15 '12 at 12:47

Dependency injection in PHP - simple example review

2 Answers2