Your DI framework is killing your code

I read a really interesting post recently looking at the difference between typical OO code and a more functional style. There’s a lot to be said for the functional style of coding, even in OO languages like Java and C#. The biggest downside I find is always one of code organisation: OO gives you a discoverable way of organising large amounts of code. While in a functional style you might have less code, but it’s harder to organise it clearly.

It’s not Mike’s conclusion that I want to challenge, it’s his starting point: he starts out with what he describes as “typical object oriented C# code”. Unfortunately I think he’s bang on: even in this toy example, it is exactly like almost all so-called “object oriented” code I see in the wild. My issue with code like this is: it isn’t in the least bit object oriented. It’s procedural code haphazardly organised into classes. Just cos you’ve got classes, don’t make it OO.

What do I mean procedural code? The typical pattern I see is a codebase made up of two types of classes:

1. Value objects, holding all the data with no business logic
   Extra credit here if it’s an object from your persistence layer, nhibernate or the like.
2. Classes with one or two public functions –  they act on the value objects with no state of their own
   These are almost always “noun-verbers”

A noun-verber is the sure-fire smell of poor OO code: OrderProcessor, AccountManager, PriceCalculator. No, calling it an OrderService doesn’t make it any better. Its still a noun-verber you’re hiding by the meaningless word “service”. A noun-verber means its all function and no state, it’s acting on somebody else’s state. It’s almost certainly a sign of feature envy.

The other design smell with these noun-verbers is they’re almost always singletons. Oh you might not realise they’re singletons, because you’ve cleverly hidden that behind your dependency injection framework: but it’s still a singleton. If there’s no state on it, it might as well be a singleton. It’s a static method in all but name. Oh sure its more testable than if you’d actually used the word “static”. But it’s still a static method. If you’d not lied to yourself with your DI framework and written this as a static method, you’d recoil in horror. But because you’ve tarted it up and called it a “dependency”, you think it’s ok. Well it isn’t. It’s still crap code. What you’ve got is procedures, arbitrarily grouped into classes you laughably call “dependencies”. It sure as hell isn’t OO.

One of the properties of good OO design is that code that operates on data is located close to the data. The way the data is actually represented can be hidden behind behaviours. You focus on the behaviour of objects, not the representation of the data. This allows you to model the domain in terms of nouns that have behaviours. A good OO design should include classes that correspond to nouns in the domain, with behaviours that are verbs that act on those nouns: Order.SubmitForPicking(). UserAccount.UpdatePostalAddress(), Basket.CalculatePriceIncludingTaxes().

These are words that someone familiar with the domain but not software would still understand. Does your customer know what an OrderPriceStrategyFactory is for? No, then it’s not a real thing. Its some bullshit you made up.

The unloved value objects are, ironically, where the real answer is to the design problem. These are almost always actual nouns in the domain. They just lack any behaviours which would make them useful. Back to Mike’s example: he has a Customer class – its public interface is just an email address property, a classic value object: all state and no behaviour [if you want to play along at home, I’ve copied Mike’s code into a git repo; as well as my refactored version].

Customer sounds like a good noun in this domain. I bet the customer would know what a Customer was. If only there were some behaviours this domain object could have. What do Customers do in Mike’s world? Well, this example is all about creating and sending a report. A report is made for a single customer, so I think we could ask the Customer to create its own report. By moving the method from ReportBuilder onto Customer, it is right next to the data it operates on. Suddenly the public email property can be hidden – well this seems a useful change, if we change how we contact customers then only the customer needs to change, not also the ReportBuilder. It’s almost like a single change in the design should be contained within a single class. Maybe someone should write a principle about this single responsibility malarkey…

By following a pattern like this, moving methods from noun-verbers onto the nouns on which they operate, we end up with a clearer design. A Customer can CreateReport(), a Report can RenderAsEmail(), and an Email can Send(). In a domain like this, these seem like obvious nouns and obvious behaviours for these nouns to have. They are all meaningful outside of the made up world of software. The design models the domain and it should be clear how the domain model must change in response to each new requirement – since each will represent a change in our understanding of the domain.

So why is this pattern so uncommon? I blame the IoC frameworks. No seriously, they’re completely evil. The first thing I hit when refactoring Mike’s example, even using poor man’s DI, was that my domain objects needed dependencies. Because I’ve now moved the functionality to email a report from ReportingService onto the Report domain object, my domain object now needs to know about Emailer. In the original design it could be injected in, but with an object that’s new’d up, how can I inject the dependency? I either need to pass Emailer into my Report at construction time or on sending as email. When refactoring this I opted to pass in the dependency when it was used, but only because passing in at construction time is cumbersome without support.

Almost all DI frameworks make this a royal pain. If I want to inject dependencies into a class that also has state (like the details of the customer’s report), it’s basically impossible, so nobody does it. It’s better, simpler, quicker to just pull report creation onto a ReportBuilder and leave Customer alone. But it’s wrong. Customer deserves to have some functionality. He wants to be useful. He’s tired of just being a repository for values. If only there was a way of injecting dependencies into your nouns that wasn’t completely bullshit.

For example using NInject – pretty typical of DI frameworks – you can create a domain object that requires injected dependencies and state by string typing. Seriously? In the 21st century, you want me to abandon type safety, and put the names of parameters into strings. No. Just say no.

No wonder when faced with these compromises people settle for noun-verbers. The alternatives are absolutely horrific. The only DI framework I’ve seen which lets you do this properly is Guice‘s assisted injection. Everybody else, as far as I can see, is just plain wrong.

Is your DI framework killing your code? Almost certainly: if you’ve got value objects and noun-verbers, your design is rubbish. And it’s your DI framework’s fault for making a better design too hard.