Code Review, Development

I’ll Show You Mine If…

Code reviews have a bad reputation.  Claims of wasting time, pedantic reviewers and hot-shot programmers all crop up as reasons why teams shouldn’t and don’t bother reviewing any of the code that is being used in what could be a multi-million dollar project.  But why would you want to have a code review?  What does it bring to the project that would otherwise be missing – because that’s the point really, if it is a waste of time then should we be doing it at all?

The Reason For A Review
Every review needs a reason.  Throwing some code in front of a group of developers without purpose will generally end up with a bunch of incoherent comments, suggestions and ideas that might be good, but depending on the project or the time scale, totally irrelevant or unfeasible.  The following are just some of the reasons you might want people to look over a set of code.  There are, of course, many more reasons but I think the majority will fall under one or more of these banners, regardless of who is doing it or what you hope the outcome will be.

Finding Bugs
It’s funny, and it might just be me, but every time I’ve asked someone to put up something for the first time, at least one bug is always found.  It might be a misplaced define, or an = instead of an == or any number of things but there’s always one.  And I know that the time it took to post and complete that review will be much less than the time it takes for that bug to be found, a bug report written, the author to read the bug, find the code in question, do some tests and then finally fix it.

Obviously not every bug will be found by a review.  It might be 1 in 10, it might be worse or it might be better.  But the point is that bugs will be found that otherwise would sneak through into QA or milestone builds, and each one will take time to find, report and fix.  Time that could be better spent doing something much more productive.

While I can pretty much guarantee that a bug will be found in the first review someone does, I would also place bets on the number of bugs being found as the reviewing continues goes down.  The number of suggestions and improvements people make will become less demanding and more of a conversation.  This isn’t because the reviewers are losing heart, or that less code is being reviewed, it’s due to one simple fact – the build is become more and more stable as more reviews are done.

This could be down to a number of reasons, but one of the biggest reasons in my opinion is peer pressure.  Programmers are suddenly aware that they are not the only people who will see this code, that their peers will be looking at and critiquing the work being done.  And most people want to look good next to their peers.  This is not a bad thing despite the connotations ‘peer pressure’ can bring to the mind.  Mutual respect with your co-workers and the need to drive yourself forward are often driven by peer pressure, and when one or two people start to move forward, a good team will always follow.

Sharing Domain Knowledge
The more people know about a particular system the better.  Illness, team moves and resignations will always be part of the work environment and it means that the key team member you are relying on could be gone before you know it.  And if you need someone else to dive in there and fix a couple of issues that have been found by QA, how long do you think that will take?

Code reviews can help.  As code is written, modified or fixed it is being looked at by more than one person.  The author has to explain their decisions, how things work and how things plug together.  Because of this you might have 2 or 3 people on your team that are happy to dive in and help.  Even if you need two developers to pair because the knowledge is spread between them, it’s still better than having no-one know what the heck that particular piece of code is doing.

Sharing Experience
People learn best when they are surrounded by people they can learn from.  Books and blogs are excellent resources (as an avid reader of books I would say that), but there is no substitute for someone sharing their years of experience with you in a 5 minute conversation.  Having people suggest different ways to structure, optimise and write code might seem a little late when it’s already written, but it’s something people can take with them into their next task, rather than going back and re-writing large chunks of code.

And it works the other way too.  There is nothing better than graduates coming into a company with their crazy ideas about how code should be written or used and teaching the old guard a new trick or two (C++? Inheritance? Madness I tell you!).

Coding Standards
This is one of the reasons people hate code reviews.  They don’t want them to turn into conversations about where the bracket should go, or if that variable should have an ‘i_’ prefix or not.  And on the whole they are right, and if this is what your code reviews turn into then you are simply ‘doing it wrong’.

But this has its place.  Developers new to the team can be given documentation about coding standards, but it is one of those things that tend to easily slip, and there is no reason why this shouldn’t be picked up in a review.  If it continues and every review becomes about how the standards are not being met, then you have an issue that should be resolved outside the review before it makes the whole process a pointless and much less useful exercise.

Problems With Code Reviews
Of course code reviews have their problems but then what doesn’t?  Most of the time these have nothing to do with the actual review process and more to do with the management of the team, or what has come before.

  • Arrogant 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 someone’s code apart or to make someone feel like they are simply not good enough.  If you start to find that one or two programmers are simply making blanket statements or not taking anything on board then this is certainly a management issue and should be resolved outside the review process.
  • Pointless reviews – This was pretty much covered in the Coding Standards section, but there is always a chance that code reviews start to only skirt the surface of the code without looking at anything to deeply.  If this starts to happen, you need to reassess what each review is for and what each person’s role is.  And remember, there is nothing wrong with simply saying “This 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 who has the last say.  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.  If they are not then again this needs to be resolved outside the review process before it goes any further.
  • Lack of time – It’s an important milestone week, code needs to be fixed quickly and you don’t have time to review it.  This can happen and sometimes you need to take it on the chin.  But isn’t this the time when it’s even more important that everything is checked and double-checked?  If you have a milestone week and your changes are coming thick and fast, then maybe you don’t have time to review everything, but why is so much changing at such a critical time?

5 thoughts on “I’ll Show You Mine If…”

  1. Hi Lee, were you talking about code inspections rather than peer reviews? I realise that most of what you said applies to both practices anyway, but I was just curious.

  2. I’m mainly talking about code inspection, whether than be checking a diff of recent changes, the review of a set of cpp/h files or complete system.

    Most of the points would be applicable to peer review of an initial set of designs or pseudo code as well.

    What would you class as the differences between code inspection and peer reviews?

  3. Pingback: jRietveld

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s