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
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.
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