Another best practice: Command query separation
I came across some code the other day that made me wish more people were aware of the concept of Command Query Separation. It’s a great default behavior for developers to have and results in code that support more reuse.
Since I think someone out there would be upset if I posted the actual code, here is something else I made up:
// bunch of other Form suff
void validate() {
List errors = getValidationErrors();
okButton.setEnabled(errors.size() == 0);
}
}
Looks simple enough. I’ve got a form, and when everything is valid, the button becomes enabled and the user can move on to something else. The getValidationErrors() method gets me a list of errors if the form has any error. Oh, but wait, it does more:
List errors = new List();
this.menu = new Menu();
if (!email.isValid()) {
errors.add(INVALID_EMAIL);
this.menu.add(new MenuItem(IMPORT_EMAIL_FROM_OTHER_ACCOUNT));
}
if (!name.isValid()) {
errors.add(INVALID_NAME);
}
if (!address.isValid()) {
errors.add(INVALID_ADDRESS);
this.menu.add(new MenuItem(LOOKUP_ADDRESS_ON_MAP));
}
return error;
}
The code’s contrived, and only loosely based on what I saw. What I saw was much more egregious. The point is that the method getValidationErrors() is building a menu and collecting validation errors.
What if I want to find out if everything is valid later on? Well, I’ll be rebuilding the menu again. It get worse when you see the code get more complex like so:
errors.add(INVALID_EMAIL);
this.menu.add(new MenuItem(IMPORT_EMAIL_FROM_OTHER_ACCOUNT));
} else {
if (email.matchesBusinessRule()) {
refreshAdvertisementAboveForm();
}
}
Sigh. I just want to know if the form is valid. I don’t want the advertisement to refresh above the form. What I really want is a getValidationErrors() and a updateMenu() and a refreshAdvertisementAccordingToRules(). Alternatively, I would have accepted a getValidationErrors() and a doABunchOfUiChangesInResponseToFormValidation().
As you may have guessed, the code that I saw was not tested either.
I have a suspicion that some developers (even some experienced developers) create code like this because they feel that they are writing the least amount of code possible (you save lines by not creating more methods!). Developer awareness aside, probably the top reason this tends to happen is lack of collective code ownership. A developer has to add a feature to a part of the system that “belongs” to someone else, so they add they try to add as few lines or if-statements as possible. They see that there is logic that they need, and they see that it is executed at the time they want it to execute their code, so they pile on their logic and finish their task.
From the example I’ve shown, it may look like a small evil, but the amount of if-else statement and total coupled logic tends to build up quick.
Tags: Best Practices
August 11th, 2008 at 5:21 pm
This kind of links into the idea of methods only doing one thing as well I think. I’ve been reading about this concept in Domain Driven Design. Refers to Side Effect Free Functions in there (page 250 onwards) -> quite an interesting read.
It seems after a couple of years of OO for me that many of the key concepts e.g. Law of Demeter, Single Responsiblity Principle, etc are all really closely linked even though they all help to improve one specific aspect of your code.
August 11th, 2008 at 8:43 pm
It is indeed related to the idea of functions/method going one thing well. That’s going to be the basis of a future post unto itself.
Many of the principles are closely related - often they are only subtly different. I dislike that any of them are called laws though. In the end, all the techniques are just ways of allowing us humans to better communicate with machines, and better collaborate with each other.
September 20th, 2008 at 6:14 am
[…] colleague Kris Kemper talks about a very similar OOP best practice called command query separation. From Martin Fowler’s description: […]
November 30th, 2008 at 12:04 pm
[…] public links >> bestpractice Giving Grants to Individuals Saved by indd on Sat 22-11-2008 Another best practice: Command query separation Saved by opentorrent on Mon 10-11-2008 Finding and Keeping Board Members - the role of best […]