Having recently finished the amazing PPP book (more here) my code-sense is getting better in putting the finger on the smells in code that make it painful for me to use. This is the story of one of them, in Buildbot.
Disclaimer: Buildbot is a pretty awesome building/continuous integration system that I use and contribute code to regularly. Indeed, it is far from perfect, but none of the coders should take offense.
So, what is the Single Responsibility Principle (SRP) ?
A class should have one, and only one, reason to change.
If a class has more than one responsibility, then the responsibilities become coupled. Changes to one responsibility may impair or inhibit the class’ ability to meet the others. This kind of coupling leads to fragile designs that break in unexpected ways when changed.
And now, to the gory details. Buildbot has a built-in class for executing shell commands, called ShellCommand. Now let’s say we want to create a class, VerboseCommand that always executes commands with the -v flag. One would assume this is the right way to do it:
But this turns out to be wrong. Say we tried to execute nosetests, the actual command that will be executed is nosetests -v -v (which is harmless, but you might have guessed my real use-case wasn’t adding the -v flag). The reason is, as the Buildbot manual will happily tell you, that every step is also its own factory.
Wait, what? Given the fact the a certain step will need to be run in multiple builds on multiple slaves, a new one needs to be created every time. But, as a user you only instantiate the step once. Turns out that behind the scenes, Buildbot will save the arguments given to the command’s constructor and call it with them again every time it needs a new instance.
This means that in our case, we must find a way to tell in the constructor whether we’re instantiating the factory or being instantiated by the factory, and add the flag only in one case but not both. My cute example turns to this mess:
(If you’re really bothered with understanding every detail, read the manual, but I’m sure you can get the gist of it without doing so.)
Now, if you’re anything like me, you’re looking at this and wondering something along the lines of “what the hell were they thinking?”. That feeling, that tingling in your code-sense, is the smell of an SRP violation.
Because this class has to also act as its own constructor, the coupling between the two actions is high, and as the definition clearly states, the class now has 2 reasons to change.
Surely, separating this to 2 different classes (the actual step and a factory) will have solved this problem elegantly. Given that this might be considered “advanced” usage I’m willing to accept that creating a factory will be optional. But really people, this addFactoryArguments screams HACK.
If you liked this post, you’ll definitely want to read the PPP book.