Consider this method which registers a visitor to an event.
public class Event {
public void registerVisitor(String name, String phoneNumber) {
if (name == null || name.trim().isEmpty())
throw new IllegalArgumentException("Name was empty");
if (!PHONE_NUMBER_PATTERN.matcher(phoneNumber).matches())
throw new IllegalArgumentException("Number invalid");
// Do actual registration...
}
}
As a good member of society, it uses guard clauses to verify its pre conditions, because:
- It protects the method from sloppy callers.
- It communicates what we expect and makes for a crude form of documenting pre conditions.
- It helps us discovering bugs early and makes sure we do not fill our events with bogus visitors.
- In fact, even security may depend on this, as we can use guard clauses to protect us from various forms of injection attacks.
That’s all good.
Duplication is the root of all evil
At least everything would have been good if the above example would have been the only place were we had to use these guard clauses. But of course, there are other methods through which these name and phoneNumber values pass. Each of these other methods also want to verify their pre conditions and make sure they are properly called. The solution? Simple, we add guard clauses to these methods as well!
That’s were things start to go south.
Rather quickly, we live in a soup of guard clauses as we duplicate them over and over again.
Well, we’ve got a solution for that, don’t we? After all, we know the DRY principle and have even memorized the Extract Method short cut. We’ll simply extract each guard clause to a separate method and reuse these methods where they are needed. (And we’ll just put them in that “util” class which we’ve got laying around anyway.) Problem solved!
public class Event {
public void registerVisitor(String name, String phoneNumber) {
Util.verifyNameNotEmpty(name);
Util.verifyPhoneNumberIsValid(phoneNumber);
// Do actual registration...
}
}
public class Util {
// Remember to use these wherever names or phone numbers
// needs to be validated!
public static void verifyNameNotEmpty(String name) {
if (name == null || name.trim().isEmpty())
throw new IllegalArgumentException("Name was empty");
}
public static void verifyPhoneNumberIsValid(String phoneNumber) {
if (!PHONE_NUMBER_PATTERN.matcher(phoneNumber).matches())
throw new IllegalArgumentException("Number invalid");
}
}
Problem solved? Well, not quite.
Sure, we got rid of some of the duplication – but not all. We still have to invoke these guard clause methods. So we still have the validation spread out everywhere. And that is assuming we remember to add them at all! Subtle security bugs may arise because we forgot to add a single guard clause in a single place.
That’s not very good.
Thankfully, we can do much better.
Understanding the problem
To get there, we’ll start by thinking about the code that we are duplicating. Why do we have to put the “name not empty” guard clause all over the place? What does the places where we add it have in common? The answer is quite simple; they all have a name parameter.
The pain we’re feeling is actually a symptom of that we’ve separated logic from data. Because after all, where would be a more obvious place for the name validation logic than together with the name?
Unfortunately, the name is currently just floating around anonymously in a String, so we cannot move the method to the data. (The static guard methods are a sign of that.) But we can solve that!
Value objects to the rescue
The solution is to introduce a value object Name which can hold both the data and the validation logic (and the same for PhoneNumber). It would look like this.
public class Event {
public void registerVisitor(Name name, PhoneNumber phoneNumber) {
// Do actual registration...
}
}
public class Name {
public Name(String name) {
if (name == null || name.trim().isEmpty())
throw new IllegalArgumentException("Name was empty");
this.name = name;
}
//...
}
public class PhoneNumber {
public PhoneNumber(String phoneNumber) {
if (!PHONE_NUMBER_PATTERN.matcher(phoneNumber).matches())
throw new IllegalArgumentException("Number invalid");
this.phoneNumber = phoneNumber;
}
//...
}
Now we’re in a much better place!
- The
registerVisitor()method reads much cleaner. It is absolutely clear what the indata is, and we can go straight to understanding the actual domain logic without being distracted by guard clauses. - Each guard clause has a single place to call home. It no longer needs to be spread out all over the system, nor does it have to live in some “util” class. It can live together with its data, as it should.
- This point becomes even more important if the validation logic is more complex than in this rather simple example.
- You do not have to remember validating the incoming data. You know that the data you get is sound. Save for the use of black magic (reflection), it is simply impossible to create a
NameorPhoneNumberobject in an invalid state. - The new value objects are excellent places to put any logic regarding these concepts that might show up in the future.
- And for “usual suspects” such as
toString(),equals(),hashCode(), andcompare().
- And for “usual suspects” such as
- You can control whether and in what ways the object can be mutated. In this case, we used an already immutable String, but in other scenarios the underlying data structure may very well be mutable.
- We’ve made previously implicit concepts explicit. No longer is the meaning of “name” and “phone number” some fuzzy things that we all think we share common definitions of. Instead, they are explicitly defined concepts, clear and self-validating.
The only possible drawback I can think of is generating more objects which might occupy more memory. However, for every case where you haven’t got a profiler in front of me telling me it’s a real world problem that particular situation, I’ll argue it’s worth it.
What do you say?
Thanks to Daniel Deogun whose talk on writing secure code triggered the writing of this post.
Let’s say we have an important class with both high complexity and many dependencies. It is really in need of unit testing because of its complexity and importance, but is hard to isolate properly. Trying to unit test such a class typically leads to long tests that are hard to read and maintain, and that need a lot of complex set-up.