The discussion around using Null is a long one. I wrote before about “Why Optional is better than Null”, but there is more to discuss regarding using Null as a way of expressing meaning.
If you are doing OO programming, you should not be using Null as you drink coffee in the morning.
In that other post on this subject, I focussed more on the common case where Null is used to express failing computation. Another way of using Null is by making it a way of expressing meaning. If you are doing that, stop it. There are proper, better ways of doing it.
Let’s take a look at this example: You want your users to send a request to another user, who either approves or rejects it. I have seen such cases being coded like this:
What is wrong here? The problem really is that the Boolean attribute hasBeenApproved, by being null, holds meaning and business logic.
Furthermore, note that the setDecision method gets a primitive boolean since the responder can either approve or decline, not ignore it. Again, such logic is mapped in a hidden, implicit and weak manner.
Now, imagine that the request could be ignored… It would require a new boolean attribute (wasIgnored) to be added, which would only check that specific case… ugly!
By last but not least, this object is not shy. Even as an outsider you easily guess how it works inside, which is not good as that causes unwanted coupling, not only around types but also around an implementation.
This code would be used as follows:
It doesn’t read nicely. It sounds very procedural and it’s not straightforward what
true means there.
By checking the method definition, you know straight away how it is implemented inside the class.
Handling new requirements
Now your client has new requirements: The Request responder must now be able to not only decline or approve it, but also mark it as Need of Work.
I guess this would change the code by creating first a RequestResponseType:
And then adjusting the Request to:
This is getting dirty. Every change will cause the Request object to expose more of its internals, making the logic around it become spread instead of contained.
It could be used like this:
Ugly…! Note how this approach breaks a few principles:
- encapsulation: Request exposes its internals
- coupling: An outsider knows about RequestResponseType
- Tell, Don’t Ask: rather than asking an object for data and acting on that data, we should instead tell an object what to do (Martin Fowler)
Doing it better
It is true that following the way that is shown above does work. “But it works” is not good enough. And I wouldn’t be happy to maintain such code after three or four new requirements have arrived and turned it into cooked spaghetti.
In order to avoid breaking those principles listed above, I would refactor the previous approach into something like this:
How does this help?
- The logic around “Awaiting Response” and around setting the response is no longer ugly and implicit.
- With this approach, Request no longer exposes its internals. One doesn’t know how the different response/statuses are mapped and implemented internally.
- Decreased coupling as there is no external knowledge of ResponseRequestType.
- Now one Tells Request, instead of asking for data.
Note how different the code is used, now that the knowledge and decision logic lays where it belongs.
This improved version is not great yet. The more statuses are introduced, the bigger Request will become. And if Request needs to include more data such as date, author, etc, this class no longer will respect the Single-Responsibility Principle. Furthermore, one can change the status of the Request, which may not be desired.
I might write a post later on how to improve it, so keep posted.