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.
Exhibit A:
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.
Exhibit B:
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.
Exhibit C:
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?
In the line 657, the constructor creates a LocalPluginManager, a subclass of the class PluginManager, passing the current Hudson class instance. In the line 667, Hudson constructor calls initTasks():
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.
Exhibit D:
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.
|
|
This is always an interesting area of debate. My personal take (and the take of I believe the Sonatype guys) is that yes the existing Hudson/Jenkins code base will start to become _very_ hard to extend unless some code debt is cleaned up first.
But hey, lets help out! The developers have created an awesome app that many of us use. So why not only take the 20-25 minute exercise, but then submit your solution as a patch
.
Cheers,
Martijn (@karianna)
Perhaps the authors have concluded that they ain’t gonna need it? Then, before adding additional code, they plan on doing the additional 15 minutes of refactoring to make it easy to extend.
That proves some things :
1. there is no link between good code and success
2. there is always possible to make some code better
3. sometimes you can loose clean code in favor of fast development
On the point of “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.”
I’d just like to point out that this er.. style.. of code is probably closer to the norm (in the grand scheme of things) than having “clean code” is. I’m sure everyone has had their very own depressing moment when they first see a systems code-base, and that cliché rain-cloud appears above their head
J.
I agree, the real world is rarely clean. Sometimes I think it’s a myth – its extremely rare to non-existent in software.
The Hudson project is currently undergoing refactoring to be more testable and tested in terms of coverage by automated testing. I am sure a bunch of patches from you would be very welcome.
Interesting. Regarding code quality, I have generally observed that there are more examples of clean code in open source projects than in for-fee proprietary code — and that this seems to be result of “good enough is good enough” principle for latter. So in a way this would be a counter-example for more common cases.
But one thing worth considering is this: you can’t directly correlate code quality and success of a single project — naive way to put it would be “but think of how successful project would be if it was cleanly built”. One would have to compare similar projects, or have larger samples. Individual data points are still interesting and thought-provoking of course.
The way I see it, bad code (~= technical debt) usually stifles future potential, adding friction over time; new things become harder and harder to add (esp. without breaking existing features). And without reasonable unit test coverage that can become nightmare. Yet I agree in that for specific case of this project, it doesn’t seem to have had too much negative effect; it does not seem likely that the project would have been significantly more succesful, or was slowed down excessively due to code stink.
I guess most practically tho the thing is that there is ugly code, wrong code, but what matters most is where said code is. Examples do look pretty bad, but it sounds like such vestigal code is somewhat harmless; in area(s) that are not modified often, nor have functional defects. They may be time bombs (one day some combinations of development can lead to actual issues), bit like tiniest cancerous growth can reside in body for long time (possibly your whole life) without adverse effects; this because immunosystem manages to keep them in check. Same can be true for isolated bad code.
Finally, the nicest thing of writing open source stuff is that even if it was the case that code goodness played no part in project’s success, I can completely afford to write good code. I choose to write good code because I like both doing it, and enjoy the results. At work I can not always do that — lots of practical limits and trade-offs guide one towards just-good-enough solutions; and the usual refactor-it-later is redirected as well by corporate moves. But at home, well, I can spend as much effort as I feel like for taking care of aspects I care about. And since quality is important for me (more so than popularity or even success of the project itself), I can ensure it gets covered.
This could also play part for hudson/jenkins, as it is not “just” labor-of-love project, but it is counted as bread winner for many participants: focus is heavily on being succesful as measured by other criteria.
When did Hudson see the light of day? some 2006?
It’s not surprising that a system that old, has gained a lot of technical debt. It didn’t seem to start with testing and grew from that.
How does the code you wrote 2006 look like? Did you use TDD or tests at all? as far as I know thats 3 years into the TDD movement. Given the learning curve for TDD it wouldn’t be surprising if it hadn’t spread to the start of the development of Hudson.
Patches are always welcome
What’s intriguing about your post’s title is that your content doesn’t even offer speculation or opinionated conjecture as to “how it got this bad”. Instead, you choose to pin-point exhibits that fail to conform to your level of purity. And, indeed, that is your right and I respect you for it.
Having been involved with OSS for nearly 10 years, and being employed by respectable business for the same amount of time, I can easily attest that:
1. Business deadlines often constrain ideal solutions
2. Many hands making light work doesn’t always enable the production of ideal solutions
3. Long-term software is organic, as is our ever-changing industry, and requires _at least_ periodic reviews and well defined strategies in order to implement ideal solutions
4. Jumping to conclusions and emotional judgements without trying to understand historical context can cloud the ability to innovate and work towards future ideal solutions
So, all that to say, bring on the forks and pull requests to clean the puppy up.
Interesting.
But you don’t have to read all the Hudson.java code to know there is something wrong. Almost 200 lines of imports are enough to show you that. Anyway, I really hope that jenkin’s community starts to submit more and more pull requests to improve the code.
@mike I agree on items #1 and #2, but not on item #3 though, alas, I’ve done it myself more than once… but then again, who hasn’t?
“Giving up quality for speed” is actually misleading, it should read “giving up quality for the illusion of speed”: you may be faster on the short run (note the “may”), but you’re going to pay a lot of interests sooner than you would hope.
seems like a lack of dog fooding. Hudson building Hudson and automatically updating.
I have seen code this bad, and some a lot worse.
Without going into a lot of detail of what makes bad code, I think it helps us to look at why bad code is written.
I just finished a project for a client who used to be my full time employer (I worked as a regular employee – not a contractor as I did this time) working on a codebase I used to work on previously (from the outset of the codebase). So I was able to see the code again after being away from it for a while. Two things I noticed:
1) Even though I knew it was well below optimal before I left the org, now I could see that even more clearly. I learned things after leaving that org about how to write even better code – things that I didn’t attempt or think about while there because I was in heads down “get it done and survive” mode.
Before I left the org I was trying to improve the codebase, but the improvement wasn’t as much geared towards fixing existing technical debt and cruft as it was towards adding new features that the product needed and the org mandated be added. Still, from time to time I tried to cleanup any cruft and refactor code to be simpler.
2) Coming back to the codebase after several years away from it, I saw it didn’t get better. No one had made attempts at cleaning anything up. Any changed/new code ranged from complete cut and pastes of whole classes with a few lines changed here and there, to complete messes, to some new features added in an okay manner but not better than what existed.
The end result was there was more cruft, more technical debt, and more bugs. There were some new features and abilities, but that was it. And that was pretty much all the org cared about by the time I was hired again to add yet more features. The org was now literally a bare skeleton of what it used to be, and lived only off adding new features to the product paid for directly by current users.
It was interesting to see the additions by people who mostly didn’t understand the patterns used in the product, and probably didn’t care. They too were mostly temp contractors – but ones who had never seen the code before.
I tried to not make the code worse, and in that I am pretty sure I succeeded. I tried to “do no harm” and cleanup a little of each mess when I found it, if I could. But that isn’t what I was hired to do. At least what I added did not create additional cruft in the core codebase (in part because most of what I added was a separate project).
Looking back, I think the reasons for the poor code (before I left) were:
a) No real emphasis on good architecture/design/practices. Almost all emphasis was on “get it done and ship it”. I understand that any org needs to meet deadlines to survive, but you can still do that and write quality code – at least to some degree.
As the cruft/debt acquires, you pay more and more interest on it. As the “friction” increases, it gets harder and harder to make changes of any sort. Eventually you will get to a point where it is a huge struggle to do anything with code (something I experienced in a previous org where the codebase was an order of magnitude worse).
Indeed, it is my belief, that at the end, this org missed out on a contract that would have helped them survive (they did not) because they were unable to get that contract in under a price the customer would pay, because the cost of changing the code to do what the customer wanted was just too high. Properly designed somewhere along the timeline, the cost of the change could have been cut by 30 to 50 percent IMO, but it was too late – now they had to pay the interest and the customer wasn’t interested in at that price.
b) The devs on the codebase each had different ideas about how things should be done, and it showed in the code. Everybody did their own thing with varying degrees of success or failure. There was little leadership in most areas of the codebase about how things should be done.
There were and still are, huge amounts of copy-n-paste code. There were a number of different ways of doing the same thing. There was no “fluency” or coherence to the layers of the code. I’ve seen a lot worse, but the codebase was/is still a mess.
c) Finally, as pointed out above, quality doesn’t always directly correlate to initial success. If the code works well enough then people will use the product. But eventually poor quality gets in the way of making progress, or even just maintaining the current level of user quality to where people will continue to use the product.
I’ve seen some really bad code be wildly successful for a while, because it does something that people need and no one else has created a product that fills that need. But eventually someone else notices that success and comes along and competes with the original product – often they do it better and with a better quality codebase. Or maybe there is no competition, maybe the users just want more and more features, or performance or whatever, and it gets harder and harder to give them what they want.
Instead of discussing whether or not which particular piece offends good practices in which way: Everyone fork away and submit patches.
In the great Hudson/Jenkins debate, I believe making the codebase testable is one of Sonatype’s main goals in their stewardship of the Hudson codebase. see the third bullet point here: http://www.sonatype.com/people/2011/02/hudsons-bright-future/
This is one of the complicating factors of deciding whether to use Hudson or Jenkins going forward. If it’s clear that one fork is focused on introducing new features and hasn’t cared about clean code, and another fork is interested in long-term maintainability, which do you choose? I honestly don’t know the answer.
There are some very good comments in response to your article!
I thought I’d express my own beliefs (and so paraphrase a few of them…)
Any long lived piece of code is going to have many people working on it, each with their own unique style and understanding of the code base.
Every piece of functionality added is going to have it’s own motivating circumstance, so, for example, there might be time or financial limitations driving it that we, the code readers, are not aware of.
It is almost always easy to criticize others code, but without insight as to how that code was created and added to, the criticism is, to me, shallow. It perhaps gives us insight into the critics preferred coding style, and also maybe teaches us a little.
But beyond that, the code works. The code is released often. The code is very popular. I think that is the true test of software. Regardless of our personal feelings on reading the code.
You ask where does this code leave software craftsmanship? Hudson/Jenkins is the rock on which many continuous integration projects are based, so I guess it seems to show that software craftsmanship is really not as important as releasing early, releasing often, and getting immediate feedback from a wide community of users. That seems to trump code ‘quality’.
But you know what? It does look as though they could do with a little help cleaning that code base up. We should start by contributing some tests…
This just shows that it’s possible to be successful without always following best practices.
Billy Joel is pretty successful, and he didn’t finish high school. I’m not going to start telling kids to drop out of high school just because it worked for Billy Joel. If kids want success to be more likely, they’ll finish high school.
If you want your project to be successful, you can increase the odds by writing good, elegant, maintainable code. But it won’t guarantee success if your idea sucks or somebody else does it better or your timing is lousy. Conversely, bad code doesn’t guarantee failure if your idea and timing are good.
As with everything else in life, code quality is one of many, many aspects of a project that you must make tradeoff choices about. The Hudson guys made that tradeoff wisely, or at least not poorly enough to fail.
one of the reasons why tge code has ended up like this is backwards compatability.
you can take a plugin compiled against hudson 1.78 and it will still work in jenkins 1.406.
that is a good 4-5 years of backwards compatability.
yes there is a lot of cruft. yes there are bad code smells. but to do a grand refactoring without breaking that backwards compatability would not be possible (note there may be some old plugins that are broken for some newer versions, file a bug if you find one, but i have a specific example of a propriatory plugin for a former employer that we had not even recompiled since 1.78)
I think this reinforces a key point: what makes successful software is providing the functionality that your users want.
That is really hard. It takes a lot of experimenting, and going down blind alleys and listening to feedback, and then adapting. Also listening to what users say they want, and from that deducing what they really want. Open Source seems to be quite a good, if slow, way to do this.
Compared to delivering value to your users, code cleanliness is a secondary concern. It affects your ability to keep doing better things for your users in future.
Cleaning up the code later by refactoring tends to be easy to do later for any half-way decent software craftsman. You just need to be given the luxury of time.
Very good analysis. Like the first commenter, I would like to see your fix to the listed issues.
Eddy.
@Stephen Connolly
>you can take a plugin compiled against hudson 1.78 and it will still work in jenkins 1.406.
>that is a good 4-5 years of backwards compatability
Unfortunatly (from what I understand from Gojko’s post) is that it does not have the unit test to prove this.
When you have unit tests, it’s easier to stay compatible and still refatcor to keep things clean.
When you don’t have the tests, you are right, then it becomes messy.
y
Thank you for pointing out some obvious “duh” code snippets. I’ll fix those, but I’d also like to make some counter-points.
Regarding exhibit B, you missed the cleanUp() call that removes the singleton reference. So yes, a single JVM cannot have two Hudson instances at any given time, but we can still test them and we do. We have 800 or so tests (http://ci.jenkins-ci.org/view/Jenkins%20core/job/jenkins_main_trunk/), so while more tests are always desirable, I don’t think it’s fair to say “not testable at all.”
Regarding exhibit C, ItemListener.all() is there for making APIs more discoverable. When you access ItemListeners, it’s shorter and easier to remember this way. For historical reasons some extension points have been registered in other ways, so this also allows us to hide that from plugin developers.
And regarding exhibit D, a super class with a static singleton reference to subtype isn’t all that new. If you’ve used enums with methods defined in constants, you are using a super type referencing subtype singletons. It’s also common in strategy pattern implementations and in class hierarchies for AST where you commonly have some terminal nodes as singletons.
But the individual technical points aside, I’m sad that you didn’t see the parts of the code that really achieves some great functionalities, such as the way we do remote code execution without requiring identical jar files to be around on the remote slave, or the way it enables plugins to take advantage of polymorphism, which was a key for the extensibility by plugins, or the way it uses POSIX API calls where available to provide greater vertical integration. In my mind, those are some of the key ingredients of the success of Jenkins, and I’d like to think those are reasonably well written.
And boy, it sure makes me feel very down to see someone trash my work with such strong words, but it helps me keep myself humble and I suppose that’s good…
The code isn’t nearly as bad as you make it out to be. At least the variable and class names are reasonably self-explanatory, unlike many other open source projects out there.
Not possible to maintain backward compatibility? Impossible?
Nothing is impossible in software – it would just be a matter of time and effort. If the idea is that a particular plugin should work per spec I don’t see how keeping compatibility would be impossible unless a new spec is contradictory to that. Implementation is not the spec, it is the implementation of the spec.
Now if a plugin relies on or works around a bug, then yes, fixing that bug might break that plugin, but that is life in the software world.
So…. where is your patch?
To all the (obvious) Hudson contributors here:
It’s amazing how far you can get by divide and conquer. The company I work for has a server with a vast amount of code debt (nearly 10 years of it). Like the bullies we are; we decided to pick on one of the smaller components and rewrite it – all you need to do for backcompat is write a marshalling facade. Just doing that gained us about at 1200% performance boost on the most common operation on the server. Instead of accumulating code debt like Microsoft (remember back in 2006 when they stalled all development of all new features?) do something about it now.
“Where’s your patch?” He shouldn’t need to patch it in the first place; if he wants to contribute to your valuable project – he should be able to get into it without too much friction. He will go find another project for the same reason you want a patch; us developers are lazy. A FOSS project reaching contributor starvation is never a nice story.
And because Hudson and Jenkins forked some month(s) ago, does this mean you now have to patch two projects?
“The code isn’t nearly as bad as you make it out to be.”
This is not just an improper software pattern. It is actually a circumvent around proper OOP: one shouldn’t use an object before its constructor is finished. By exposing the instance in the static field, they (*) just managed to do that. OMG!
(*) Unfortunately, this is a case where a collective mind of several developers happened to produce not even proper OO code, let alone good design. This is an issue of Open Source code management. Developers, beware: the fact that it is open sourced doesn’t mean it needs no management. Not sure if the rest of the code is like this, but certainly is a flag to this project in particular.
Watch this, time well spent:https://www.youtube.com/watch?v=fEvKo90qBns
ABout lean entrepreneurship, about 26 min in: “All my code, the well written as well as the badly written, was thrown out…”
First of all, good post!
It is really interesting to see the debate springing out from this. I wonder if there would be as many voices leaping to the codebase defense if it had not been in a successful project. If it had been a failing project or a leak from a closed project, I think the comments would have been more in tune with “this only shows how important clean code is…”
The point he is making, I think, is not that there exists bad code. That would have been a trivial point to make and not a very interesting one. But he points out that doing the right thing is more important than doing it right, which is an important thing to realize and remember. I do not believe that you generally get a trade of between quality and fast deliverables. I think this project is more a case of success in spite of bad code, than because of it. I believe that doing the right thing the right way will always get you further and keep you safer.
Gojko, any response to Kohsuke’s explanation?
I think you miss out on a lot of things, you didn’t dig deep enough.
@Bilal
I didn’t really see anything as a call to respond. Koshuke’s comments show that we have a completely opposite view of testability and clean design. Which is fine, I don’t expect everyone to agree with me. 800 tests that cover a minor percentage of code isn’t testability in my dictionary. superclasses holding and using a static reference to a subclass is a crime for me, while he obviously doesn’t think that. while I wrote about clean code, he points out clever code.
In any case, I am sure I missed out on a lot of things. I covered a constructor of a single class. There are megabytes of code more to cover. But my point wasn’t to dissect every single piece of hudson, it was to point out that a project with a complete disregard for clean code is very successful.
The project is successful because it is useful, it gives benefits to the users, better than any other alternatives, as simple as that.
You can’t say “a complete disregard” when you only touched a tiny fraction of the codebase. How about saying “the classes I’ve checked out disregards…”? That would reflect the fact better.
In that case, how about if I’m calling for you to respond to Kohsuke’s explanation?
After reading Kohsuke’s comment, I got the impression that what you said in your exhibits only demonstrated your lack of understanding, as to why certain things were done that way. I’m not saying the code is clean, but your arguments were missing the point, hence it would be great for the discussion if you can respond.
@Bilal,
my arguments are supporting the point I tried to make, that code quality isn’t a measure of success.
It’s sad that you have to trash other people’s work just to prove your point, is there no other nicer way?
I understand your position though. Actually, since you’re such a proponent to clean code, and its your belief. Why don’t you stand up to what you believe in, and make an improvement to a popular product like Hudson/Jenkins. You said it would only take 15 to 20 minutes, how about you submit a patch with the clean up you suggested?
This way we can see if your belief is actually practical, and whether you, as a proponent of clean code with all those books you wrote/are writing, can back up what you believe in.
Related: http://www.johndcook.com/blog/2011/05/16/bumblebee-software/
“Almost every software engineering principle that has become generally accepted as useful and valuable, Emacs flouts. The code is 24 years old, huge, and written by hundreds of different people. By rights, the whole thing should blow up. But it works — and works rather well.”
I would fire anyone who wrote code like this without thinking twice
Really?
I have only worked in the UK, NL and NZ. In none of these countries can you fire someone for writing bad code. Your best opportunity to avoid their working style would be at interview when you ask them to show you something they have already written.
Gross incompetence is grounds for instant dismissal. This work is not incompetent since it clearly works.
If such a person already works with you, or for you, the only thing you can do is mentor the hell out of them. And hope that some of it sticks.