In my last post I covered what I see to the The Benefits of Code Reviews. But with every process there are problems that if not tackled can cause teams to lose these benefits and remove it from their development process.
So what do I see as the biggest problems facing introducing and maintaining a code review process?
Aggressive reviewers & victim syndrome
Code reviews are there to improve the code base for the benefit of the team. They are not there to ‘go on the attack’, to rip some-one’s code apart or to make someone feel like they are not good enough.
If people start to fear putting up their code for review then the number of reviews will suddenly start to drop or even worse only the most trivial of code will be submitted.
The opposite of this is the review owner who is too sensitive to any kind of code critique. Any non-positive comment is seen as an attack on them rather than the code and criticism is met with defensive comments rather than seeing it as an opportunity to learn.
Having an over-aggressive reviewer is not that difficult to tackle. Taking them to one side, reviewing their comments and how they approach the reviews will often resolve the issue quite quickly. Most of the time the reviewer won’t even be aware of how their comments are coming across and are not intending to be aggressive at all.
The later is much harder to solve.
Quite often the developer is highly protective of the code and may be more defensive when reviewed by certain developers. It might be possible to start off by restricting who reviews their code or what is reviewed, starting smaller and slowly introducing more of their code to more people over time.
A Lack of Positivity
Remember that there is nothing wrong with the only comment on a review being “That looks great!”.
Lack of Outcomes
People will argue, and people will disagree, especially when it comes to code. “You should do it like this”, “I don’t want to change that because…”. Discussions are great and should always be encouraged, but at some point there needs to be a decision.
You have to know who has the final say – whether this is a manager who is on every review, or a senior responsible for the quality of the code. A simple comment along the lines of “That’s a great suggestion for the future” can put an end to most arguments with most people being happy with the outcome.
Sometimes a short face to face discussion will bring these discussions to a suitable close for everyone involved if the discussion starts to turn into a tit-for-tat point discussion.
Lack of Follow Through
The worry here is that nothing changes as a result of the code review. Obviously not every suggestion or comment needs to be acted on but if developers are seeing review after review being posted and time and again every comment being ignored or not even acknowledged, it can have a devastating effect on buy-in.
This kind of behaviour is both demoralising and infectious.
If developers see their comments being ignored time and time again they will start to do two things. Comment less and react less.
If their comments are being ignored then why should they continue to post more comments? And if people can ignore their comments why should they respond to comments on their reviews?
Sometimes it takes time to respond to a review comment. To avoid situations of reviews just ‘sitting there’ I like to make sure there is a closure plan for all reviews. That way people know that if a review is still open but not responded too, they are still processing the information.
But if reviews are constantly being closed and the suggestions ignored, then this needs to be resolved quickly. Maybe a manager or senior developer re-opening the review to ‘bump’ the comments to make sure they are referenced might solve the problem quickly enough.
Unfocused Reviews
Sometimes people don’t know what to review so every review dawdles around the code and nothing of note ever comes to the surface. In a lot of these cases review after review will seem to focus on coding standards and nothing else.
In situations like these I like to ask people to suggest where people focus their time on the review. This won’t be on every review and it’ll only be certain developers who do this (usually those who know where their weaknesses are).
By suggesting areas for people to focus on it will start to open up both those reviews and ones that don’t have specific suggestions as people start to get more ideas as to what to look for and what might be important.
Unrealistic Expectations
There are no silver bullets for a development team. No process will take you to a communication nirvana, there is no way of working that will stop code rot and nothing you can do to stop some bugs getting through to a possible submission build.
If you start to expect that a code review process will cause every bug to be spotted before the code is submitted or that suddenly every ones skill level will rise to that of the best developer on the team then you’re setting yourself up for disappointment.
But by accepting that the review process won’t solve all your problems but that it will have a generally positive and cumulative effect on the quality of the work being produced those expectations will start to lower but your team will be better off for it.
Any More?
There are not the only reasons why a code review process can falter but from my experience they are some of the main ones. What things have happened to your review processes in the past and, most important of all, what could have avoided it?
In the next post I’m going to describe some of the different ways we’ve reviewed code and the various benefits and pitfalls they had.
Apologies if I take some time to respond to comments to this post. I’ve scheduled this to be posted while I’m out of the country and I won’t have the time or opportunity to respond until I get back. But don’t let that stop you posting! :)
mroth. Used with permission.