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) {The isValidEmail method checks the email against a simple regular expression. This snippet springs at least two different considerations.
String email = emails.get(role);
return email != null && isValidEmail(email)
: getDefaultEmail();
}
public boolean isValidEmail(String pattern) {
return (...stuff...);
}
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) {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:
String email = emails.get(role);
return isValidEmail(email)
: getDefaultEmail();
}
public boolean isValidEmail(String pattern) {
return pattern != null && (...stuff...);
}
public Email getEmailForRole(Role role) {A simplistic implementation of the NullEmail object could be
Email email = emails.get(role);
return email.isValid
: getDefaultEmail();
}
class NullEmail {If we wanted to explicitly return a String instead of an Email object we could slightly modify the getEmailForRole method:
private Role role;
public NullEmail(Role role) {this.role = role;}
public String getValue() {return "";}
public Role getRole() {return role;}
public boolean isValid() {return false;}
}
public String getEmailForRole(Role role) {As you might have noticed, nothing changed in our NullEmail class - nor in the Email class, which is not shown here.
Email email = emails.get(role);
return email.isValid
? email.getValue()
: getDefaultEmail();
}
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:
Post a Comment