If you have a few minutes to spare, have a look at the Hudson/Jenkins source code on GitHub. Writing code like this wouldn’t just be a firing offense for any self-respecting commercial software team, it should also be enough to sue the writer for gross negligence and damaging company property. Yet, this project is undoubtedly a huge success and its authors should get the respect for creating it. Where does this leave the whole debate of software craftsmanship?
It isn’t really a surprise that the test coverage is abysmal, very few opensource projects have good coverage. But this code goes far beyond anything I’ve seen. Fair enough, any big project is going to have rough parts, but check out the hudson.model package – which should be the heart of the design and the cleanest aspect of it – and in particular the heart of the heart – the constructor of the Hudson class.
I’ve forked the code so that the line numbers won’t change quickly.
The Hudson class is 3883 lines long, 134.5 kb in total. The constructor alone is almost 100 lines. Single responsibility principle applied? Only if the responsibility was to confuse any potential readers.
Hudson class is a singleton, but done according to a patterns book written by the evil twins of the Gang of Four and edited by the Hungarian Dictionary book publisher from And Now For Something Completely Different. The constructor isn’t private and the reference isn’t by any means maintained by a static factory method. Nope. The constructor is public, checks and sets the reference itself in the lines 620-621, making it effectively callable once and not testable at all.
This instance seems to be used to prevent two applications starting at the same time, but then also as a service locator passed around and accessed from all over the place.
The singleton instance is used as a service locator in places where this is entirely unnecessary. For example, in the line 640 of the same constructor, the instance accesses its own instance method getRootDir() through the static reference. WTF?
This method is defined in the PluginManager superclass, and hits the singleton directly:
So a subclass, that already has an instance of Hudson passed into its constructor, doesn’t initialise the superclass but lets it depend on a static singleton reference. I don’t really see a reason for this, the subclass could just as well pass the instance to the superclass constructor and avoid this static dependency.
In line 703, the Hudson constructor calls ItemListener.all():
That method uses the singleton reference to call an instance method on Hudson:
Why is this method in ItemListener at all? It is a classic example of feature envy, let alone that it has to use a static instance to invoke another method of the instance of another class that actually calls it. There are many other similar examples, that could all be taken out just by passing the right instance along. Or even better, methods full of feature envy should be moved to the right place.
The Node class is a superclass of Hudson, but also has several references to the static singleton instance of its subclass. For example, see the method toComputer() on line 160:
Lets go over this again. A superclass has a static singleton dependency on a subclass. How’s that for a brainmelt? And yes, this static instance is used of course during the constructor execution for the subclass.
So where does this leave us?
I’m a firm believer in clean code, and I would fire anyone who wrote code like this without thinking twice. On the other hand, I think that the people behind the project deserve respect because they are obviously making something successful, and I certainly respect them for that. I am not really sure where this leaves us. The fact that this code works, let alone that the project is a success, is close to the final proof that God doesn’t exist for the whole craftsmanship debate. It will be very interesting to see if Hudson/Jenkins is going to collapse under its own weight in the near future, or will it continue to gain popularity.
For homework, fork the code and try to refactor it to prove that the singleton isn’t really needed in all those places that use it from the constructor. Try to get the class constructor under unit test, for example. Put some tests around it and using automated refactoring to bubble the singleton references up to the Hudson constructor, then replace them with instance references. It is good fun and takes 15-20 minutes.
I'm Gojko Adzic, author of Impact Mapping and Specification by Example. My latest book is Fifty Quick Ideas to Improve Your Tests. To learn about discounts on my books, conferences and workshops, sign up for Impact or follow me on Twitter. Join me at these conferences and workshops:
How to get more value out of user stories
- Stockholm, SE, 16 October
Specification by Example Workshops
- Stockholm, SE, 14-15 October
- Vienna, AT, 2-3 November
- Kraków, PL, 17-18 November
- Tallinn, Estonia ,3-4 December