How we Achieved the Best Code Quality in my Career
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.
Clear Architecture
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.
Code Reviews
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.
People
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.
Pia Fåk Sunnanbo on 2012-12-20
Very interesting!
I am pretty curious about the “People” section of your post. Were the developers in project Beta hopeless cases, or do you think that that project could have been saved by the experience from project Alpha if you’d have to do it again?
Anders Abel on 2012-12-20
I wouldn’t say that the people in project Beta were hopeless cases, but there were problems in that team that we didn’t have in project Alpha. Getting the project Beta team to succeed would be possible, but it would set very high demands on the technical leadership. Especially those devs in Beta that lacked motivation to improve, or sometimes even lacked understanding of their own shortcomings would have required a much more firm architectural lead to make the project succeed.
Pia Fåk Sunnanbo on 2012-12-20
I would also like to know more about your experience of code review practices. Are informal peer reviews enough or is there a need for a formal approach? What is an optimal number of reviewers? Are review meetings where several people discuss the code worth the time spent?
Anders Abel on 2012-12-20
In project Alpha we reviewed all code. That calls for a very streamlined process. We had no meetings, but rather a review queue that a senior developer would look at before starting at a new task. The developer doing the review would ask for a second opinion from another senior developer if needed. Most feed was handled as written comments on the tickets, to keep the entire review-feedback process async. This was an optimisation we found early in the project. If a review is to be done face to face, the developers are interrupted in the middle of coding. That’s bad for productivity.
For our project, a senior developer reviewing everything was perfect as it helped us catch problems with the junior developers’ code in time. In a team were most developers are senior it’s maybe better to take another approach, focusing on more formalized group reviews for select key functions but leaving the vast volume of the code without reviews.
What’s important to keep in mind though is that reviewing great code takes no time. If the code is well written and properly documented a normal commmit can be reviewed in about 1 minute. It’s when there are issues with the code or with the documentation that the review takes time. In that case it will take even more time to fix the bugs if they are found during testing or even worse, after the code has gone live.
Pras on 2012-12-21
I think you got lucky with the type of “people” and “management” mix you had. Sometimes with the right mix of people things just work out
Tom Straight on 2012-12-23
Hi,
Very nice article. I am also interested in your code review practises. Is the code committed to source control & then would a senior developer review the commit or do you have a staging area of some kind?
Anders Abel on 2012-12-25
During the project we tried a couple of different ways. At first we decided that the code should be reviewed before committing to source control. It’s a great way to make sure that bad design doesn’t affect (read: infect) more of the code. Unfortunately it has a great drawback: The senior developer doing the code review has to drop whatever coding he/she is working on to make reviews whenever another developer is done coding. Until getting the review the developer is effectively stuck doing nothing so reviewing must get high priority. With doing reviews as my highest priority I never got any concentrated coding time myself so I had to change how we worked.
During the remainder of the project we allowed the non reviewed code to be committed into source control. The to-review code was kept track of in the issue tracking system. We linked each commit to a specific coding task and the task was marked as ready for review. When reviewing a task, the diffs of each of the commits belonging to the task were investigated. If everything is fine with the code, such a review takes a couple of minutes for a medium sized task.
For us, it was fine to have non-stable code in the main development branch since we were not yet in production. A team working on a system that is in production and maybe even makes use of continuous deploy would of course require a more complex source control handling with a staging area/branch.