Wednesday, March 30, 2011

Duplication is evil, double duplication is worse

Think of an application with a domain model in which a person can have several emails, each of which has a different role (e.g. private, office, preferred, and so on). Persons can choose their preferred email for normal communications, but there are other communications that are normally sent to the email with a given role (e.g. communications about personal health will not be sent to an email which is also read by secretaries); only when an email with this role is missing the system reverts to a default email, which is chosen between the existing ones applying a chain of rules.

In an application like this you could (hypotetically speaking, of course) find a snippet of code that returns the email corresponding to a given role:
public String getEmailForRole(Role role) {
String email = emails.get(role);
return email != null && isValidEmail(email)
? email
: getDefaultEmail();
}

public boolean isValidEmail(String pattern) {
return (...stuff...);
}
The isValidEmail method checks the email against a simple regular expression. This snippet springs at least two different considerations.

Check for correctness

One might think that the check for validity is unnecessary, after all checks are done while inserting and updating, right? Partially. Let's just say all checks should be done there, and let's not forget that data greatly outlive applications, so you could easily have strings with completely different meaning stored where only emails should be. Sounds ugly? Welcome to reality. Sure, you could quite easily find all instances of strings that are not emails, but when the customers asks you to leave stuff as it is you have very little power. Other things I've seen include courses descriptions instead of teachers and documents instead of relatives. To cut a long story short, better safe than sorry.

Be clear on intents

Stick with them and hide mechanics. In particular, here we have a duplicated duplication (pun intended):

  • the check for null values must be duplicated in every snippet that contains emails.get(role)
  • the check duplicates the intent of the isValidEmail method. I think this duplication is worse than the previous one, first of all because the redundancy blatantly hits the eye, but most of all because there is a conceptual duplication: the responsibility for the check should lie in the idValidEmail method itself.

If there's an isValidEmail(String pattern) method I expect it to check for null values without having to bother myself:
public String getEmailForRole(Role role) {
String email = emails.get(role);
return isValidEmail(email)
? email
: getDefaultEmail();
}

public boolean isValidEmail(String pattern) {
return pattern != null && (...stuff...);
}
A more elegant solution could be the creation of an immutable Email value object and the use of the NullObject pattern, a special case of the more general Special Case pattern (another pun intended); in this case the emails object would return a NullEmail object that implements the same interface of the Email class:
public Email getEmailForRole(Role role) {
Email email = emails.get(role);
return email.isValid
? email
: getDefaultEmail();
}
A simplistic implementation of the NullEmail object could be
class NullEmail {

private Role role;

public NullEmail(Role role) {this.role = role;}

public String getValue() {return "";}

public Role getRole() {return role;}

public boolean isValid() {return false;}
}
If we wanted to explicitly return a String instead of an Email object we could slightly modify the getEmailForRole method:
public String getEmailForRole(Role role) {
Email email = emails.get(role);
return email.isValid
? email.getValue()
: getDefaultEmail();
}
As you might have noticed, nothing changed in our NullEmail class - nor in the Email class, which is not shown here.

Of course in this particular case we still have to check for the validity of the email, but in many other places (e.g. print emails of a person for every possilbe role) we can safely operate on our NullObject just as we would on a valid one.

It could be overkill, but it surely gets rid of one of the worst plagues in software: duplication. But that's the subject for another post :-)

No comments: