Code Reviews that might actually work

June 4, 2012

I’ve had an opportunity to be part of a team doing a lot of greenfield development on a new codebase at a client recently, and it’s been a lot of fun. The client already has a codebase that’s grown organically over a decade to meet the changing and complex needs of a highly successful company, and is in surprisingly good shape considering. Still: it’s huge, incorporates several competing implementations of The One True Programming Style, the occasional flash of mad genius, and a lot of code that was written by very dedicated developers working very hard to make very tight deadlines.

The new codebase shares none of the constraints of the old one, and the team is keen to keep things as pristine as possible as long as possible. One of the best tools in our arsenal is the enforced code review. New code entering the codebase needs to have been reviewed, no exceptions – and the goal is that most of the team reviews each piece. So far it’s working out spectacularly well.

LinkedIn tells me I’ve been working on teams that have tried to incorporate code reviews with varying degrees of success for almost 6 years now, sometimes in larger teams that were sat in the same office, sometimes in smaller ones that were internationally distributed, and sometimes when I’ve paid external developers to look at code I’ve been writing by myself for clients.

So, here’s what seems to work:

Formally make time for it

Few people seem to enjoy code reviews. There’s the mental effort of understanding what someone was trying to achieve, the cognitive load of understanding how a piece of the system you’re not working on is meant to fit together, and it takes time away from the joyful process of actually programming.

Conveniently, people will often stop harassing you to do them if you claim to be too busy. This has the downside that with the best intentions, code reviews stop getting done, or just get cursory glances and rubber stamps.

So we’ve instituted Review Time: 30 minutes in the morning before the standup when if someone asks you to do a code review you’re obliged to, as your top priority. If you have a piece of work outstanding that needs review before you can merge it in, you know that your team mates are available to get that done for you, right then.

As tickets in our issue tracker can’t be marked complete until they’ve been reviewed, developers bug other developers to get the reviews done, and have a time of the day when they know no-one can claim to be too busy…

Have a time of day when code reviews are everyone’s top priority

Get code reviewed by as many team members as possible

Code reviews have several collateral benefits that mean the more team members who review a given piece of work, the better.

Firstly, it’s a great way to make sure development isn’t happening in personal silos. If you have an “ActiveMQ guy” and a “Stats Engine lady”, you’re going to find yourself in trouble when the AMQ Broker loses its breakfast while the AMQ guy is on holiday, or when there’s a lot of Stats Engine code to be written, and the Stats Engine lady is snowed under with other tasks. It allows other developers to identify where the parts of the system that aren’t obvious to them are, and thus where and how more documentation is needed.

Secondly, code reviewing is a great opportunity for mentoring within the team. A senior developer reviewing a junior’s work is going to be able to suggest avenues for improvement the junior might not have thought of – use of a particularly library or feature, or areas where there are subtle bugs or side effects.

Equally, a junior reviewing a senior’s work may get exposed to ideas and techniques that are non-obvious to them – whether that’s a house style, a useful idiom, or a workaround for a problem they weren’t even aware of. They’re also more likely to ask disruptive questions that may not have a good answer about why the senior developers are doing things a certain way – it’s too easy as a senior developer to assume that other senior developers have good reasons for some of the stranger code they write, and not question them…

When you consider the collateral benefits, getting code reviewed as widely as possible in the team makes a lot of sense

Use a review tool, and add accountability

Use a Code Review tool – it adds accountability, encourages detailed reviews, and archives useful commentary (we’re using Review Board).

As we discussed above, the actual process of writing code reviews isn’t always that much fun, and there’s a temptation for it to turn in to rubber stamping – “uh, yeah, that looked fine” isn’t usually a useful review. The act of putting your name on a review and saying “Ship It”, knowing it’ll be saved in the review system for all time can help focus the mind a little.

If the code breaks and the original developer isn’t around, it’s easy to look up which developers reviewed the code, and thus should be able to answer questions on it. This tends to make developers much keener to do a thorough job in the code review – both in making sure they understand the code, and in being confident the code is well-tested and of high quality.

My experience has been that code reviewing also becomes a lot easier with a decent tool – you can annotate pieces of code, start discussions with other reviewers and the original author in one place, and easily track improvements to the code as a result of the review.

Finally, it archives useful discussion. While ideally everyone would be producing and keeping up-to-date formal documentation of their architecture, style, and implementation decisions, the real world doesn’t always works like this. Being able to drop in to git blame to see which commit some changes were made in, and then being able to go and read the discussions about it at the time can give you much better insight on why certain things were done a certain way, even when the documentation is a little thin.

Use a review tool. Really.

Keep a checklist of things to review

Make sure the team knows what they need to be looking for, at a minimum. Code quality and coding standards need their own article, but some pointers on what people should be looking for not only help inexperienced reviewers, but also help give the original developer an idea for what they should be aiming at.

As a starting point, perhaps consider:

  • Code needs to be tested, and reviewers should be able to get the tests passing on their machine without help from the original developer
  • Enough documentation should be included that the reviewer should be able to explain how the code works, and why it was written in that way
  • House style should be followed: no crazy new indentation style, database table naming conventions, or using $thingy as an identifier…

Decide and document as a team what’s important to review

Avoiding drama

Programmers are rarely entirely egoless. Whether it’s the senior developer who’s been trying to get HR to change his job title to Scientist Guru, or the junior developer who avoids asking questions because they don’t want to look ignorant, people can be quite protective of their work and defensive when someone suggests they’ve done it wrong – especially if that work was particularly arduous to produce.

Decide beforehand as a team how anal you want to be about things like code style (suggestion: dial it up to 11), what a useful level of automated testing and documentation looks like, and where (for example) certain tasks fall in the MVC split. If a reviewer (or a developer) can appeal to the higher authority of the team’s best practices document, it can help to diffuse situations where one party feels they’re being unfairly targetted.

The more of the team involved in a discussion, the more the discussion becomes about how the team wants to progress than individual personalities. Don’t allow senior developers to pull rank (and if you are one, try and resist the temptation) – if someone can’t explain their decision to the rest of the team – regardless of the technical ability of the other team members – it’s often a good sign that they’re clinging to a decision they made from an emotional rather than a professional perspective.

A useful way forward with differences of technical opinion is to try and agree a general principle that the team should be following, codifying that, and seeing if it can be applied to the situation at hand.

Recognize that code reviews can be drama flashpoints, and plan around that as a team

Some final thoughts…

If you’re working well together as a team, code reviews should bring up minor issues, rather than major issues – sensible standups help you avoid nasty surprises like big architectural decisions gone awry, and general agreement and team buy-in about what constitutes good work gives everyone a standard to work towards.

Most teams have someone who can help resolve ‘tie-breaks’ where the “right way” comes down to a judgement call – whether that’s the Technical Lead on the project, or, if they’re one of the disagreeing parties, a senior dev from another team. Simply the process of explaining it to a third party is often enough to make the right way forward clear to all involved.

Split up code to code review in chunks that are logically useful, rather than that necessarily correspond to certain tickets or commits to the source repository. If your code review tool takes git commits, judicious use of temporary branches, cherry-picking, rebasing, and the path separator (eg: git diff master..mybranch -- foo.txt bar/ lib/foo.pm) can be helpful.

In Summary

Code reviews are a great way of keeping code quality high, intra-team mentoring, and making sure everyone in the is familiar with the wider codebase. Key points:

  • Have a time of day when code reviews are everyone’s top priority
  • When you consider the collateral benefits, getting code reviewed as widely as possible in the team makes a lot of sense
  • Use a review tool. Really.
  • Decide and document as a team what’s important to review
  • Recognize that code reviews can be drama flashpoints, and plan around that as a team
  • Split code in to the most useful chunks for review, rather than being beholden to your SCM or issue-tracking tool

{ 8 comments… read them below or add one }

Larry Gasik June 5, 2012 at 3:34 am

I really do like code reviews. I never thought of code review tools though. They seem really interesting to me. At my current place of work we do not do code reviews. However, I try to put lots of notes, and marks in my code for when the next person comes in. Tags like SMELL, NOTE, and REVIEW are all very common for me. They’ve also helped other developers see what I’m trying to do.

Reply

Greg June 5, 2012 at 10:09 pm

The quote “if someone can’t explain their decision to the rest of the team – regardless of the technical ability of the other team members – it’s often a good sign that they’re clinging to a decision they made from an emotional rather than a professional perspective” — perpetuates a common myth, namely that emotional thinking is not professional. I’d urge you to read a short little book called “How We Decide”. It might change your thinking on this.

Modern neuroscience suggests that often emotional reactions are pattern-based judgements from years of experience where we know something is a good or bad idea but can’t codify the reason. It turns out that in experienced experts, these judgements can be *better* than well-articulated ones.

Anyway, I’d suggest both reading the book, and keeping in mind that an emotional response from an experienced team member can be worth exploring to find out what the relevant experience is, and whether that experience is important to you.

Reply

Peter Gfader June 6, 2012 at 8:49 am

Great post!

Have a time of day when code reviews are everyone’s top priority

+100. Important to make this a habit like the Daily Standup

Use a review tool. Really.

Curious how you use a tool to review code. Just annotations for code? How does this work. We use Tags like HACK, TODO, ASK and REVIEW but not a tool…

Reply

Peter June 6, 2012 at 9:07 am

Mostly we’re using code annotations, but this allows us to have threaded conversations, open issues inline, and so on. There’s room for further discussion on the piece as a whole, too. We’ll usually discuss the architectural approach of what we’re doing before we’re implementing it, so it’s rare that we’ll have something huge come up.

Reply

Brice Richard June 6, 2012 at 12:27 pm

I work developing small-scale software projects. I’ve been developing software for nearly 12 years and I can tell you that I don’t agree with code reviews. At the end of the day your code either works or it doesn’t – it’s a boolean perspective.

I personally have no interest in having someone scrutinize the hell out of what I’m doing. I would rather more INFORMALLY discuss my code with someone and then get feedback on functionality, coding protocol, etc.

But to have someone literally read over and judge my code is not a collaboration in which I would EVER welcome.

I’m an authority in the area in which I specialize. I am continuously learning so for me, if I am going to improve as a Software Developer, it will be through reading online and offline materials, or having informal conversations with other developers about code – it won’t come from a code review.

Reply

Dave Cross June 14, 2013 at 11:07 am

“At the end of the day your code either works or it doesn’t – it’s a boolean perspective.”

Firstly, it’s not *your* code. It’s the team’s code. So it makes sense for the team to have a say in how it’s written.

And secondly, whilst it’s obviously nice if your code works there are other things that you should also be taking into account. Primarily, how easy to understand is your code?

Code reviews are about ensuring that code is written in a way that the whole team are happy to put their name on. If that seems onerous to you, then I’m just glad you’re not on my team.

Reply

me June 6, 2012 at 8:50 pm

Heh. I think Brice might be the “drama flashpoint” this article is talks about.

Reply

DeanWombourne June 7, 2012 at 8:47 am

It’s interesting that brice’s view of a code review includes the word Judge. If you don’t like the idea of a formal code review you can make them more informal – its your call!

It’s also interesting that Brice doesn’t seem to see any benefit in letting others ask about his code – if I were in the presence of an authority, I’d certainly enjoy being able to ask directly why things were done a certain way.

And, as one of the more senior devs in my team, I still enjoy code reviews _because_ of the questions that people ask me. I’m often stumped for an answer when a junior asks ‘why have you done A not B’ or ‘What about C?’ I’m often presented with new ideas and I’ve never has a code review where I have wanted make no changes to my code afterwards!

My advice to Brice is

1) Don’t assume the reviews are all about you. Everyone will gain from them.

2) turn your informal chats into a more regular thing – it sounds like you’re doing code reviews already in all but name and regular scheduling!

3) Don’t (like I did for a few years) assume that people with less experience can’t teach you anything.

Reply

Leave a Comment

Previous post:

Next post: