Clean Code is maintainable
Source code must be:
1) Readable
2) Extensible
3) Testable
We are going to stress on the most neglected of these attributes.
To test a method:
1) we need to instantiate an object (system under test) and all the helpers(mock providers, stubs) etc. but it turns out that we put lot of code in constructor which makes it very difficult for create object of it.
Suppose we have a pest call in our constructor. Now while testing, we actually have to make a pest call for instantiation process.
class Story{
public function __construct() {
$pestObj = new Pest();
$this->content = $pestObj->get('http://www.getStory.com');
}
}
But instead of this, if I ask for what I need :
class Story{
public function __construct($pestObj) {
$this->content = $pestObj->get('http://www.getStory.com’);
}
}
What I can do now is, while testing, I can mock get method of $pestObj and we wont actually have to make get request.
Now everything looks alright but look at this for a moment:
1) Story knows about a pest client and makes a GET request just for its content.
How about this:
class Story{
public function __construct($text) {
$this->content = $text;
}
}
and
class StoryStore{
public function __construct($pestObj) {
$this->pest = $pestObj;
}
public function getStory(){
return new Story($pest->get('http://www.getStory.com'));
}
}
This helps because tests are about instantiating small portions of application and run them and assert some output.
And if it is difficult to instantiate an object its not just going to be difficult to test that object but also other parts that need that object.
Its a kind of transitive problem.
1) So test case has to successfully navigate the constructor each time the object is needed.
SO constructor ideally should be very simple. Just simple assignments and we are good to go.
Service Locator (context, service container):
In this system we pass the responsibility of creating very hard to instantiate objects to service locator and then in code we pass this service locator around.
class MyController extends Symfony\Bundle\FrameworkBundle\Controller\Controller
{
public function testAction()
{
$this->get('my_service')->func();
}
}
and Symfony Controller does implement ContainerAwareInterface, and has a service locator role.
But the problem with service locators is that they encourage the APIs that lie.
You can't tell from looking at the definition of a class what its dependencies are because they aren't being injected,
instead they are being yanked out of the service locator
Now we need to test a class that only says I need a service locator.
But we have no idea what all we need to make the test run. we have to see the source code.
what all are needed to run the class or see where it crashes and what needs to be over ridden.
and most of all it violates the law of demeter.Law of Demeter also known as principle of least knowledge is a coding principle, which says that a module should not know about the inner details of the objects it manipulates.
If a code depends upon internal details of a particular object, there is good chance that it will break as soon as internal of that object changes.
(Always ask for the things/references you need instead of searching for those references.)
According to Law of Demeter, a method M of object O should only call following types of methods :
Methods of Object O itself
Methods of Object passed as an argument
Method of object, which is held in instance variable
Any Object which is CREATED locally in method M
More importantly method should not invoke methods on objects that are returned by any subsequent method calls specified above
and as Clean Code says "talk to friends, not to strangers"
-> Only Ask for objects you DIRECTLY need to operate on.
-> $a->getX()->getY() is a no no
-> $serviceLocator->getService() breaks law of demeter
->Dependency Injection of specific objects we need.
Also if we want to reuse the class that require service locator then we have to supply client with all the compile time dependencies which include service locator.
But service locator contains many other services and dependencies.
So here we have a problem, in order to give a class we need to give service locator and in order to give service locator we need to give most part of our application.
Breaking the Law of Demeter is Like Looking for a Needle in the Haystack
public class XMLUtils {
public function getFirstBookCategoryFromXML(XMLMessage $xml) {
return $xml->getXML()->getBooks()->getBookArrary(0)->getBookCategory();
}
}
This code is now dependent upon lot of classes e.g.
XMLMessage
XML
Book
BookCategory
Which means this function knows about XMLMessage, XML, Book and BookCategory.
It knows that XML has list of Book, which in-turn has BookCategory, that's a lot of information.
If any of the intermediate class or accessor method in this chained method call changes, then this code will break.
Its an understanding between you and code how to traverse the haystack and get the thing you need.
All the things in between are irrelevant all u need is the book Category
Fundamentally this is what's wrong with service locator, asking for things we don’t directly need but retrieving the needed from it.
In summary, abandon the use of new operator.
-> new operator ideally should either be inside factory or in tests.
But in some cases it is perfectly OK like instantiating a HashMap(), a collection . No need to ask for it in the constructor.
This is because they are the leaves in your application graph. There is nothing behind these. Its the end of the road.
But for services, objects that need collaboration, those need to ask for their dependencies.
Construction should be done either by DI container (symfony container), or builders or factories.
Any kind of indirections we introduce in the code makes it very hard for writing tests.
This cost becomes double the cost if this is done in constructor. As now it has to be shared by lots and lots of the tests that use that specific object.
Writing Testable Code
Flaw #1: Constructor does Real Work
Warning Signs
- new keyword in a constructor or at field declaration
- Static method calls in a constructor or at field declaration
- Anything more than field assignment in constructors
- Object not fully initialized after the constructor finishes (watch out for initialize methods)
- Control flow (conditional or looping logic) in a constructor
- Code does complex object graph construction inside a constructor rather than using a factory or builder.
- Adding or using an initialization block
Flaw #2: Digging into Collaborators
Warning Signs -
- Objects are passed in but never used directly (only used to get access to other objects)
- Law of Demeter violation: method call chain walks an object graph with more than one dot (.)
- Suspicious names: context, environment, principal, container, or manager
Flaw #3: Brittle Global State & Singletons
Warning Signs
- Adding or using singletons
- Adding or using static fields or static methods
- Adding or using static initialization blocks
- Adding or using registries
- Adding or using service locators
Flaw #4: Class Does Too Much
Warning Signs
- Summing up what the class does includes the word “and”
- Class would be challenging for new team members to read and quickly “get it”
- Class has fields that are only used in some methods
- Class has static methods that only operate on parameters
So now we know how to write UN TESTABLE code and wreck havoc on person writing UTs:
- Make Your Own Dependencies – Instantiate objects using new in the middle of methods, don’t pass the object in.
- Heavy Duty Constructors – Make constructors that do lots of work in them.
- Depend on Concrete Classes – Tie things down to concrete classes – to avoid interfaces wherever possible.
- Conditional Slalom – Always, always, feel good when writing lengthy if branches and switch statements. These increase the number of possible execution paths that tests will need to cover when exercising the code under test.
- Depend on Large Context Objects – Pass around ginormous context objects (or small ones with hard to construct contents).
- Use Statics – Statics, statics everywhere!
- Use More Statics – Statics are a really powerful tool to bring TDD Infected engineers to their knees. Static methods can’t be overridden in a subclass (sometimes subclassing a class and overriding methods is a technique for testing). When you use static methods, they can’t be mocked using mocking libraries
- Use Global Flags – Why call a method explicitly? Just like using flag to set a flag in one part of your code, in order to cause an effect in a totally different part of your application
- Use Singletons Everywhere – Why pass in a dependency and make it obvious when you can use a singleton?
- Look for Everything You Need – By Looking for things you are asserting your objects dominance as the object which knows where everything is. This will make the life of tester hard, since he will have to mimic the environment so that your code can get a hold of what it needs.
- Utils, Utils, Utils! – Code smell? No way – code perfume! Litter about as many util and helper classes as you wish. These folks are helpful, and when you stick them off somewhere, someone else can use them too. That’s code reuse, and good for everyone, right? Be forewarned, the OO-police will say that functionality belongs in some object, as that object’s responsibility.
References:
1) https://youtu.be/RlfLCWKxHJ0
2) http://en.wikipedia.org/wiki/Law_of_Demeter
3) http://www.slideshare.net/theojungeblut/2013-106-clean-code-part-i-design-patterns
4) http://nschoenmaker.nl/2013/11/defining-symfony-2-controllers-in-two-ways/
5) http://misko.hevery.com/2008/07/24/how-to-write-3v1l-untestable-code/
