Sunday, March 23, 2014

Sorry Uncle Bob - but NO!

I just came across this article and was very shocked by the code and so I decided to travel back in time to read the original article by Robert C. Martin and I have to say:

I sincerely have to disagree with you, Uncle Bob!

Let me explain why

While he is trying to separate DI container code from his application logic - which is a good thing but not necessary if you design your architecture without a certain DI container in mind (as Mark Seemann writes in his comment to Martins post) - he does one ugly mistake. He introduces a static factory to his BillingService class.

This thing is nothing else than a service locator call in disguise!

While he shows how nicely you can test the BillingService class he does not show how hard it will be to test the class that contains the logic he commented with "Deep in the bowels of my system." earlier in his post. Because this is where things start to get really messy. In this code he reaches into the BillingService.factory and uses it to create a new BillingService instance which in his real application will call into his DI container and retrieve a fully functional BillingService instance. But how are you supposed to test this? The constructor of DeepInTheBowelsOfMySystem obviously does not tell you anything about BillingService or a factory. This a case when the API lies to you - and in this case really badly. Since you not only call into some dependency that you might pass into your SUT. No you call into some static member and we know that global states are bad, right? This means for a test to succeed we need to setup the BillingService.factory which we will encounter after running into a null pointer exception (or access violation).

This code is as bad as calling into a service locator at this place to get a BillingService instance.

How to solve this problem?

Either pass in a BillingService into your DeepInTheBowelsOfMySystem class or pass in a BillingService factory. Both are things that can be done with "Poor Man's DI" and with most Dependency Injection Containers. Like Spring4D supports factories out of the box. If you register something like this:

myContainer.RegisterType<IBillingService, TBillingService>;

Then the container will automatically resolve something like this:

constructor Create(const billingServiceFactory: TFunc<IBillingService>);

When you see this constructor you immediately know what dependency it expects and it does not take you couple of runs or reading through the code to find out what you need to setup for a test. The SUT should tell you what it needs and nothing more.

As you can see it is very easy to step into traps that lead to hard to test code and it's not always trivial to solve this - so even knowledgeable people like Robert C. Martin might step into these.