My recently finished project produced the best code quality in my career so far. The key success factors were a clear architecture, insane compiler warning levels and last but not least code reviews.
The project was a greenfield project so there was no legacy code to inherit which gave us the opportunity to create something great, but of course also the opportunity to screw up completely. In fact, I was involved in a similar project some years ago that had the same opportunities but came out with all but great code quality.
In this post my recently finished project has the codename Alpha,
while the other project with not-so-good code quality has codename Beta.
Alpha and Beta Similarities
The settings of the Alpha and Beta projects were quite similar. They had about the same time frame, budget and team size. The systems were built for similar organizations. Both projects set out with a high level of ambition, but while project Alpha sustained in keeping that level to the end of the project, Beta slipped behind badly. I’m proud to have been part of Alpha’s success. I prefer to not think of my contribution to Beta’s nasty, messy and stinking code.
The first success factor of project Alpha is the clear architecture. I might be a bit biased here as I was the architect of Alpha, but this is my blog so I can write that my own architecture is great and nobody can object.
Wait, that’s not true. I do allow comments on the posts… I’d better explain myself.
The important word in clear architecture is not architecture but clear.
A clear architecture is extremely important for any greenfield project. For the Alpha project the basic building blocks were ASP.NET MVC3, Entity Framework 4.3.1 Code First with migrations and jQuery UI. Based on those building blocks an architecture was defined with the project split up in a few different assemblies and each assembly in namespaces. The key success factor is that there was an overall plan for how the assemblies and namespaces were defined and what functionality should go were. It doesn’t matter very much what the overall plan look like. What matters is that it exists and that it is clear and communicated to all developers.
Looking back at project Beta a couple of mistakes were done. The first was design by committee. I do believe in a democratic design process where all opinions are taken into account. The problem is when the resulting architecture is a compromise. That’s what we got in project Beta. Several architectural decisions ended up as compromises between two alternatives effectively getting all the disadvantages of both alternatives and none of the advantages.
The second big mistake was the lack of enforcement of the architecture. In all projects (including Alpha) there are times when some developer on the team hasn’t understood the architecture and writes code that doesn’t fit in. In Beta, there were no routines for finding out and correcting those issues before it was to late. In Alpha we fixed it in time, thanks to the code reviews.
Insane Compiler Warning Levels
Both projects set out with a bold ambition and enabled all code analysis warnings available. In project Alpha we soon realised that some of the warnings were not relevant for us so they were disabled quickly. The first thing that got disabled was the entire internationalization block since this application is custom built for Swedish use.
In project Alpha we never had more than 3 or 4 warnings in the checked in code base. Those warnings were mostly that freshly created namespaces contained too few types. If the namespace wasn’t filled with more types it was removed and the types in it merged into another namespace.
Other warnings were corrected or sometimes suppressed in the code if there was a good reason for it.
In project Beta people simply ignored warnings they didn’t see as relevant. The warning list grew over time and was soon impossible to handle. The last time I looked at it a full rebuild issued about 4000 warnings. Some of them minor code analysis warnings, some of them major issues such as incorrect if statements giving unreachable code.
Last, but definitely not least on my list is code reviews. Early in the Alpha project we decided that code reviews should be part of our Definition of Done (DoD). Whenever a developer in the project had coded and tested a task it was not marked as “Done”, but rather as “Ready for Review”. I and another senior developer on the team monitored items in the “Ready for Review” state and looked through them. Often there would be comments and things that needed to be improved. In that case the item was sent back as “In Progress” for the developer that had done the initial coding. That way all developers got immediate feedback on their own work and had to fix any issues in their own code.
That of course included myself and my code too. I had to go back and fix naming, documentation comments and move shared logic to helper methods to get my code approved. The reviews helped both junior and senior developers to improve code quality.
I cannot emphasize the code reviews enough. Thanks to them several cases were people unknowingly were solving the same problem twice were found and refactored into a common solution. Thanks to them we found security issues. Thanks to them we found performance bottlenecks that would strike once the system has production volumes of data. Thanks to them we got a common style and naming convention in the code. Thanks to them relevant discussions on design and architecture regularly took place. Thanks to them we got a collective ownership of the code.
I don’t think I have to mention that there were no code reviews at all in project Beta. The code produced differed vastly in style and quality depending on which developer had written it. When it was bug fix time it was long forgotten who wrote what code, so there were no feedback to each developer.
The Key to Great Code Quality
The one key success factor to great code quality is communication. The architectural discussions and code review discussions in project Alpha is what kept all code aligned to the architecture (and sometimes made us change the architecture to fit reality). In project Beta there were not enough of those discussions and they often didn’t end up in any clear decisions.
Now I’m just about to start a new project. One of the project Alpha devs that will also be involved in the new project told me last week that he really wanted us to do code reviews in the new project too. That dev has actually sometimes had a hard time with my reviews, where I have sent back items two, three or four times before approving them. Still he thinks that the reviews are valuable. That is a sign of a true will to improve, a true will to learn.
In project Alpha everybody wanted to learn and improve. In project Beta people were happy that things compiled. The key to great code quality is communication, but that will only ever take place if the developers do want it, if they see it as a chance to learn rather than a risk of getting caught writing ugly code.
Starting my next project I look forward to the reviews. I know that they will be a great source for learning, both for me and the rest of the team.