Last week, I had a discussion with Ionut – my colleague at Ninespices – about a piece of code I had wrote in one of our applications. The piece was meant to log the current user’s actions to the database for future reference. It looked something like this (more or less, the actual code isn’t that important):
protected function _log($info) {
$identity = Zend_Auth::getInstance()->getIdentity();
$this->save(
array(
'user_id' => $identity->id,
'info' => $info,
)
);
}
Ionut said to me: “I don’t like it! It’s hardcoded!”. I said that it’s not, as I don’t hardcode anything, since we all know that each time one hardcodes something in a software application, God kills a kitten.
But Ionut asked me a simple question: “How do you test it? How do you mock Zend_Auth?”. For the record, Ionut is the Test Driven Development guy of the team. He always writes tests for his applications, whereas I don’t, since my code “just works”…well, kind of…sometimes
. The point he made is interesting: how do you mock Zend_Auth? The answer is obvious: D’oh, you don’t!
In order to be able to properly use unit testing to test a piece of code, that code must be written in a manner that allows unit testing. To make a method “testable”, you must not instantiate other objects, call other classes’ static methods and so on. For instance, let’s consider a piece of code that looks like this:
public function methodThatRequiresTesting($input) {
// get data on the current logged user
$identity = Zend_Auth::getInstance()->getIdentity();
$userId = $identity->id;
// instantiate a helper class
$helper = new MyLibrary_Helpers_Foo();
// do whatever
}
It’s quite obvious that this code isn’t “unit testing friendly” as you cannot mock the helper or Zend_Auth. The proper way to write it in order to be “unit-testable” is either:
public function methodThatRequiresTesting($input, $userId, MyLibrary_Helpers_Foo $helper) {
// do whatever
}
…or…
public function methodThatRequiresTesting($input) {
// get the user id
$userId = Zend_Registry::get('userId');
// get the helper
$helper = Zend_Registry::get('Helpers/Foo');
// do whatever
}
This way, one can easily add mocks instead of the actual objects, isolate the functionality of this particular method and test it. Of course, the first “testable” code is wrong, as it breaks separation of concerns. A method should have input and output, and any additional entities (functions, classes) used inside the method’s body in order to compute the output from the input should be encapsulated within the method. Receiving these entities as input is a very bad practice, for obvious reasons.
The second code example is better from an architectural point of view, but it can be quite memory intensive, as one might instantiate a lot of helpers and place them in the registry without ever using them, thus wasting valuable system resources.
That very night, Ionut & I went to the 26th edition of Wurbe, where one the the topics was – beside the crappy beer – Test Driven Development. There was a somewhat heated debate on whether to use TDD or not, unit testing, functional (system) testing and so on. I particularly liked one of the opinions expressed there – by a guy who’s name I forgot
– on functional testing. There’s no need to write unit tests for all your components. Only write tests for those components which contain complicated algorithms, so each time you refactor the code and optimise the algorithm, you know if what you did is good or not. And write a system test, in the end, to test the whole application.
It makes more sense to me this way.
Thanks for the “TDD guy” mention, Tudor. I feel quite flattered to be honest.
I prefer the first solution from the two above. Nevertheless, if you want to not pass the user ID and the instance of that helper every time you call that method, then maybe you should pass them as arguments to the constructor and make them internal properties.
And there are some reasons for which I don’t like the Registry solution, one being that it is deceptive. You may call that method in a test without adding those two keys to the Registry, actually without requiring the Registry at all in your script. You’d get some errors about missing key or something. Then, you’d have to require Zend_Registry and finally add those two keys.
It’s a useless dependency somehow. I’d go for the constructor arguments instead.
I still keep to my idea that an object should encapsulate the components it needs in order to function properly. I generally favour composition, not aggregation of objects, because if you change the way an object works internally, substituting an aggregated object for another, if you use aggregation extensively, you’ll find yourself in need to make a lot of changes throughout the application.
For example:
class Example { $this->_fga = null; $this->_sga = null; $this->_tga = null; public function __construct($fha, $sga, $tga) { // first helper aggregate $this->_fga = $fga; $this->_sga = $sga; $this->_tga = $tga; } } // usage - imagine its used in several > 5 places // throughout the application $fga = new MyFirstHelper(); $sga = new MySecondHelper(); $tga = new MyThirdHelper(); $object = new Example($fga, $sga, $tga); // and now imagine you have to use another class // instead of MyFirstHelper - not pretty…whereas if you use composition…
class Example { $this->_fga = null; $this->_sga = null; $this->_tga = null; public function __construct() { $this->_fga = new MyFirstHelper(); $this->_sga = new MySecondHelper(); $this->_tga = new MyThirdHelper(); } } // usage $object = new Example();In this context, that task is simpler to achieve. And it can be “unit testable”, if you start from the “inner components”. You can test the My*Helper classes first, and if they’re okay, you can test the outer – “Example” – class.
Ideally, one should have only one place to manage application dependencies.
Issues differ from app to app, but in your first snippet above, Example seems to be a singleton object, so I’d just instantiate it in the bootstrap of the application and pass it around.
The second snippet contains a problem which the first one does not suffer from. If Example is no longer a singleton and you need two instances of it (one using MyFirstHelper and the second using MyFirstHelper2), then troubles appear. You’d probably have to introduce a conditional inside the constructor that uses a different helper based on a parameter you also have to introduce, hence you have to modify all places where you have instantiated Example.
Regarding testability, instantiating classes in your constructor basically prevent you from unit testing that class. You may test it from an integration point of view, indeed, but this dependencies build-up to the extent that you’ll have to run the whole application in order to test it.
Encapsulation does not necessarily mean that a component does not receive arguments, but rather that the public interface exposed by the component is independent of the receiving arguments. Having the arguments implement a (duck-)interface solves this problem, IMHO.
Encapsulation doesn’t mean that a method shouldn’t receive any arguments. But I make some distinction between input as data meant to be processed and input as objects that help processing the data.
Anyway, in the end, everybody’s right
This post reminds me of a podcast about functional programming. The main advantage of functional programming (as I remember it) was that it is highly testable. Most methods are functions, which means they receive all the data they needs as parameters and they always return the same result given the same data. Funny how these things seem to converge…
@Siderite, indeed, there are a lot of functional programming principles that I apply to object-oriented programming. It’s something that I call functional object oriented programming.