Dependency Injection Breaks Encapsulation

tony_marston404 · May 6, 2015, 6:09pm

Those were not my words. I simply quoted them to emphasise the use of the word can which does not carry the same weight as should.

I have already told you multiple times that in those places where I chose not to use DI it was because it would have been like using a sledgehammer to crack a nut. Instead of a sledgehammer I chose to use a simpler alternative, and that alternative just happened to be a singleton.

TomB · May 6, 2015, 6:11pm

But again, how are you measuring “Simpler”. Please refer to my earlier post:

You must explain how you are measuring “better”, “appropriate”, “practical”, “intended for” or any other ambiguous term you are using to perform any kind of comparison (See section 3 of my post where I did this)

Add “simpler” to that list. I’d say simplicity = fewer lines of code. In which case DI is simpler, as I demonstrated here:

tony_marston404 · May 6, 2015, 6:13pm

I have already stated several times that I do not have any automated tests. That does not automatically make my code untested or untestable. Many successful applications have been released without having automated tests, so they should not be treated as a “requirement”.

TomB · May 6, 2015, 6:14pm

tony_marston404 · May 6, 2015, 6:17pm

Nothing can be simpler than writing a single line of code, which is what my use of a singleton entails.

Michael_Morris · May 6, 2015, 6:31pm

[quote=“tony_marston404, post:515, topic:113596”]
I have already stated several times that I do not have any automated tests.[/quote]

I see. Well, at least at long last the truth is out and in the clear.

Sorry, but no. The presence of unit tests is the definition of tested code.

No tests === no testing.

Hand testing just doesn’t cut it anymore. Drupal 8 has approaching 100,000 tests with nearly 1 million assertions. If you do each of those by hand at a minute per test without rest it will take you 70 days to run one test. If Radicore is all the bees knees you claim it to be it’s at least the same size as Drupal 8. Even if it’s magically half the size and requiring half the tests that’s still 35 days to thoroughly test by hand.

You can’t be thoroughly testing your code. It’s flat out impossible to make the claim that you do.

You are either completely ignorant of proper software engineering practice or deliberately lying - which is it? Many successful applications – bull. PHP itself? unit tested. Every major library in Ubuntu Linux distro? unit tested. Every single component of Windows? Unit Tested. I could go on until I’m blue in the face.

However, you made the claim, so name one, just one successful application that has no unit tests. Successful being defined as having an installed user base of at least half a million.

You won’t be able to do it. Your claim has become as absurd as saying the sky at high noon on a cloudless day is purple, grass in summer is red.

Misguided opinion is one thing - lies quite another. Sitepoint should not be a platform for you to disseminate blatant falsehoods that can damage the development of novice programmers.

s_molinari · May 6, 2015, 6:35pm

If you don’t understand the irony of your own reply, then I certainly can’t explain it to you.

Scott

TomB · May 6, 2015, 6:38pm

Finally a real answer! The metric you are using is number of lines of code added to use an existing dependency. Why did this take you 500 posts to explain despite being asked directly dozens of times?

But now you’re comparing apples to oranges. You’re comparing an existing singleton implementation to setting up DI. If we want apples-to-apples we need to compare an existing DI implementation to an existing singleton implementation.

Let’s take a step back and consider this class:

class Person {
	private $foo;

	public function __construct() {
		$this->foo = 'Bar';
	}

	public function method1() {

	}

	public function method2() {

	}

	public function method3() {

	}
}

and consider we need to add the address dependency to each method that needs it:

class Person {
	private $foo;

	public function __construct() {
		$this->foo = 'Bar';
	}

	public function method1() {
		$address = Address::getInstance();
		$address->whatever();
	}

	public function method2() {
		$address = Address::getInstance();
		$address->whatever();
	}

	public function method3() {
		$address = Address::getInstance();
		$address->whatever();
	}
}

Using a DI container and using the same base class, I just change the constructor and I can access the property in each method:

class Person {
	private $foo;

	public function __construct(Address $address) {
		$this->foo = 'Bar';
		$this->address = $address;
	}

	public function method1() {
		$this->address->whatever();
	}

	public function method2() {
		$this->address->whatever();
	}

	public function method3() {
		$this->address->whatever();
	}
}

What can be simpler than not even writing a single line of code to locate the dependency from elsewhere?

Of course you can do the same with a singleton:

class Person {
	private $foo;

	public function __construct() {
		$this->foo = 'Bar';
		$this->address = Address::getInstance();
	}

	public function method1() {
		$this->address->whatever();
	}

	public function method2() {
		$this->address->whatever();
	}

	public function method3() {
		$this->address->whatever();
	}
}

“Simpler” here is really negligible, the singleton approach doesn’t have a constructor argument, but the singleton has an extra method call. However, the downsides of the singleton I listed here far outweigh the effort of writing the constructor argument.

So really “simpler”? Perhaps “As simple” by this measure. What it’s not, however, is any reason to favour a singleton over DI.

felgall · May 6, 2015, 7:18pm

That’s why you need to consider the purpose for which OO was created in the first place - to break up the code into small easy to manage pieces that contain a complete definition of how a particular object is supposed to function. OO is hierarchical because trying to define everything at once means you’d need a brain the size of the universe to be able to understand it - so you define a hirearchy of classes that allow the individual parts of the more complex objects to be defined in terms of more specific ones.

If you write OO code without it being able to be easily understood by looking at it one class at a time then you are misusing OO. If a printed copy of a single class cannot be placed on your desk without the pages having to overlap then you can’t see the whole code for the class at the same time and the class is too big and needs to be redefined in terms of smaller classes. If the code can’t be broken up into manageable pieces that can be worked on independently of the rest of the code then it doesn’t matter what techniques you are using - you still effectively have unmaintainable code (you may be able to maintain it for a short while because you wrote it and can remember what it does but no one else will - including you if you come back to it in 20 years time).

The reason for all the disagreements about exactly what a particular programming style or construct is are because people concentrate too much on how those constructs are defined and overlook the fact that they were all designed for a purpose. Where the purpose for which a construct exists applies then using that construct is appropriate and you use it in a way that fulfills its purpose in order to gain that benefit. If you use a construct in a way or for a purpose for which it wasn’t created then you do not get the benefits that it was intended to provide and that will eventually cause problems.

s_molinari · May 6, 2015, 8:44pm

Nice post. But you forgot one thing. Tony doesn’t have that problem.

Scott

felgall · May 6, 2015, 10:15pm

YET.

Jasmine · May 6, 2015, 10:25pm

Everyone - be civil to each other. Do not pick on each other. If you continue to do so, you’ll be suspended for a few days.

Think about how you’re responding in this thread, how this could look to colleagues, potential employers, and what it says about your overall reputation should others find this thread. Is this how you want to treat fellow web developers?

saamorim · May 6, 2015, 10:56pm

@tony_marston404 your last response to my message made me even more confused about the organization of your dependencies, let alone out the could be injected. And I’m not going to take effort in understanding it…

So, bringing the subject again to the table.

Could we always implement DI? There are some cases that if prefer to not touch than to just trying to start injecting dependencies, for one reason my boss/arquitect/senior college won’t let you, others it could break the logic of the app, others it might not even paid the risk and it’s better to just start over.
But, Applications can be built without DI, that is a fact. I’m in charge of the maintenance of dozens of legacy apps, and half of them are OO, most of the don’t have DI when they should. But honestly, after I learned the benefits of DI, I can’t remember if I ever question the use of it, and I try to refactor it when I can. Of course, it hurts the few times but with a little of lubricant, persistence, trial and error you’ll soon get the grasp of DI, FactoryServices / Providers, and other good paradigms. It does make you code clean and reusable. I admit, sometimes is hard to get the real flow of the app because you’ll be probably using interfaces instead of classes, but you’re not oblige do so.

@tony_marston404 mentioned “I only have one choice so I won’t use DI” but the past shows me that the software requirements do change, even the most basic ones (like the php7 constructor), makes me realize that it can be true that “today that I only have one choice” but there’s great probability that tomorrow won’t be.
So, if DI doesn’t give that much work (and I do believe is has a minor impact) it is preferable to incorporate right from the beginning that feature than to postpone it.

s_molinari · May 7, 2015, 7:32am

Firstly, I am going to thank everyone in this thread, even Tony, for the debate. I personally have learned a lot. That being said, I’d like to get back to talking shop. (and either I will be showing my own ignorance or what I have learned. If I say anything wrong below, please do correct me (that doesn’t go for Tony…LOL!)

Tony, I believe you twisted (once again, sorry Aunt Jasmin ) a point in the article. The author was making rules for creating testable code and the rule you twisted is, entities and value objects are newable. He doesn’t say, newables are not injectable.

Examples of Entities could be Account or User. Entities are newables. Just create them as needed in tests or production code (where the 'new’s would typically be done in a Repository, which are very briefly discussed below).

The author also says this.

An injectable is a class that should not be instantiated as needed in the code. That is, instances should not be created with the new operator on an as-needed basis. Instead, an injectable should be injected (typically via constructor injection) in its client [hence the name]. In other words, an injectable will be supplied to its client (usually as a constructor parameter) rather than having the client being responsible for constructing or locating this dependency.

From what I can tell in this code

class Person {
    function doStuff () {
        ....
        $objAddress =& singleton::getInstance('address');
        $address = $objAddress->getPrimaryAddress($customer_id);
        ....
    } // doStuff
} // end class Person

the address object is “mixing the concern of application instantiation with concern of application logic”. This is generally an issue for testing, because we can’t instantiate the Person without automatically instantiating the Address object, meaning, we can’t test the Person class in isolation. This “no-no” is well demonstrated in Miško Hevery’s article about the new operator in unit testing, which the article you linked to, also links to.

So in end effect, the Address object is very much an “injectable”, despite it being an entity, because the use of the singleton mixes instantiation with logic.

For testing purposes, the class would be better off like Tom pointed out way back when, doing this:

class Foo {
	private $bar;

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

	public function doStuff() {
		$this->bar->x();
	}
}

And I am going to go even further out on a limb and say, there is a missing repository pattern and it is being made up for with singletons, which is simply a bad practice, when writing testable code. Note in that article the pros and cons for a repository pattern/ class.

Pros:

Separation of concerns; the application need not know about or track any or all data sources.
Allows easy unit testing as the repositories are bound to interfaces which are injected into classes at run time.
DRY (Dont Repeat Yourself) design, the code to query and fetch data from data source(s) is not repeated.
Cons:

Adds another layer of abstraction which adds a certain level of complexity making it an overkill for small applications.

For enterprise ready frameworks the added layer is necessary, simply in order to keep sanity.

And I am going to bring in another quote from that same article, because Tony, I feel you actually don’t understand what testable code is.

This is the quote.

In summary, when I have experienced testing pain personally or have seen it elsewhere, it is in general because of one or more of the following:

a lack of use of Dependency Injection

a misunderstanding of or misuse of Dependency Injection

not separating classes cleanly into newables and injectables

injection of injectables into newables

inappropriate injection of newables into injectables

disregard for the Single Responsibility Principle

I think your framework is afflicted with most of these attributes and is thus, not at all testable, as is a lot of legacy code. It also makes it difficult to consider legacy apps in current theoretical discussions, because of how OOP has progressed over the years.

Scott

Post edited by cpradio to make it sound less personal

saamorim · May 7, 2015, 7:57am

And I’m not the only one that state this:

http://www.tonymarston.net/php-mysql/role-based-access-control.html
Some people may say that having to maintain such a complex database is overkill for such a ‘simple’ requirement, but in my long experience the customer is always dreaming up ‘little additions’ to his requirements that cannot be satisfied with a primitive design. How easy would it be for you to change your current system to incorporate the following ‘little requirements’
(…)
I have also found over the years that by having such a database I can easily extend it to incorporate new requirements or features.

TomB · May 7, 2015, 8:37am

This is completely true but it needs a bit of an explanation. A testable application is not the same as testable code and are very different. Testing the application means running the entire thing as a user would and seeing if it works as intended. Testing the application is a very inefficient way of testing the code. Imagine testing a checkout process, each time you would need to enter your address/delivery information in the form to test the order was placed correctly and each time the client calls up and gets you to change the way it works (Extra discount types, voucher code additions, etc) you will need to go through this entire process. With unit testing (Testing the code, not the application) I write test and can run it over and over without needing to type all that information into the form each time. This tests the code, not the application.

Post edited by cpradio, updated the quote to reflect new verbiage and made it a bit less personal

s_molinari · May 7, 2015, 8:42am

Yes. Good point. We are most definitely talking about testable code here.

Scott

Posted edited by cpradio, updated quote

saamorim · May 7, 2015, 8:44am

This is a whole new topic of discussion and not a “simple” one. But yes, they are indeed different things and they target different audiences.

cpradio · May 7, 2015, 11:36am

Off Topic:

Anyone interested in starting such a discussion? Maybe title it: “What qualifies as testable code?”

Jeff_Mott · May 29, 2015, 4:58pm

I’m going to return to the original topic and reverse my original position.

Let me first share a (slightly edited) exert I read this morning from “Programming: Principles and Practice Using C++”:

Consider this nonsense function:

int do_dependent(int a, int& b) // messy function; undisciplined dependencies
{
    int val ;
    cin >> val;
    vec[val] += 10;
    cout << a;
    b++;
    return b;
}

To test do_dependent(), we can’t just synthesize sets of arguments and see what it does with them. We have to take into account that it uses the global variables cin, cout, and vec. That’s pretty obvious in this little nonsense function, but in real code this may be hidden in a larger amount of code.

Any dependencies that our functions and classes have, whether global variables or singletons, are by definition not encapsulated, because those dependencies exist external to our class. If we want to use a function or class correctly, we have to consider not just its formal parameters, but also any external values it relies on (aka dependencies). DI has the benefit of making sure those dependencies are obvious, rather than hidden deep inside some monolithic codebase.

tl;dr DI doesn’t break encapsulation because dependencies are, by definition, external and therefore already not encapsulated.