4th November 2013

Code review

Having written last about knowledge sharing I almost immediately wanted to get some thoughts down about code reviews. In the back of my mind during that post I was mulling over the idea of serendipitous knowledge sharing, or at least knowledge sharing that isn't of the standard documentation style (wiki, issue tracking, code comments). And so... Code Reviews!

It's hard to recommend, encourage or enforce team members to keep abreast of latest wiki posts because it doesn't fit into the usual flow of work. Yes they are absolutely necessary and useful but they are often more go-to reference rather than regular read. This may be less so if a more story telling style blog is also available but it's hardly something you'd list in your standup as a task for the day "oh yes and I'm going to read the company blog".

So if we look at a 'standard' day for a dev team member it might include a standup, writing some code on your current task, possibly helping out a colleague or maybe doing some research or breaking down some requirements for an upcoming project. Where in there can we get some cross team knowledge sharing? well, short of pair programming the only place is when you're helping someone else with a problem. But that is less controllable and often relies on proximity, friendships and people who consider themselves most available.

Endorsed code conversations

This is where code reviews come in. I'm sure you've done or are doing code reviews. To be honest I'd like to rename them 'code conversations'. Not just because, in a touchy feely way, I don't want to be combative (although that is a valid reason) but because I want to reassert the focus.

What code reviews should not be:

• A formality
• A means to fill in copious amounts of forms or spreadsheets
• A point scoring mechanism to lord it up over your colleagues
• A metric generator for who's best at writing code 'first time'
• A laborious process of checking against a style guide - this should, if done at all, be done by automated tools such as StyleCop.

Remember: My problems understanding your code are rarely to do with whether you put opening brackets on one line and far more likely to do with your overall design. Yes, if it's awfully laid out it will make it harder, but perfectly indented, capitalised and bracketed CRAZY-ASS code is still crazy code. Death by a thousand cuts is a possibility but it's avoidance isn't the end goal of a code review.

Code reviews should:

• Be Collaborative
• Provide a guaranteed, regular timeslot for the discussion of code
• Become one of the means by which you organically permeate knowledge and consistency across the team
• Be a way of injecting a social aspect to a work related activity

Code reviews should be a discussion (whether they are done in person or remotely). Crucially they are a discussion regarding an artefact, unlike design meetings or sprint planning there is a 'thing' that is the product of constraints, newly identified knowledge and a whole number of cumulative decisions. There's a lot of scope in that to get into some detailed worthwhile discussion.

In fact if you've learned nothing from a code review you're either a liar or you did it wrong. Now, what you've learned could range from a specific new programming technique to knowing more about a client you are yet to work for. It could even simply lead to you knowing one of your colleagues a bit better - which is definitely still a good thing if it helps next time you need to collaborate.

I'm not saying code reviews negate the need for design discussions or even pair programming. But code reviews are held in the cold hard light of day, objectively and ideally with a fresh set of eyes that wasn't involved in the design or coding. The aim is to remove bias and ego from the equation.

A quick note on style guides - although coding style guides don't just focus on syntax, if you've got a mature team and still need a style guide document that is referred to in code reviews then it's probably a warning sign. Focusing on syntax is the easy way out/lazy mans code review which should, if at all, be completely automated.

I haven't got all day

I'm not saying that this enforced rubber duck programming should be days long and lead to the entire codebase being rewritten but it can, and maybe should, lead to some refactoring notes or tasks. It's never 'too late' for feedback from review. Afterall your team is constantly learning, growing, evolving isn't it?

Code Review isn’t just useful for finding defects. It’s a great way to spread information about coding standards and conventions to others as well as a great teaching tool. I learn a lot when my peers review my code and I use it as an opportunity to teach others who submit pull requests to my projects. Phil Haack (emphasis mine)

If it helps, think of it as avoiding a build up of knowledge debt.

Process

Now, although I've used 'enforced', 'regular' and 'guaranteed' up to this point 'code conversations' can still be rather informal and don't have to lead to, nor should, generate reams of paperwork. Documenting counts of 'issues found' as part of a code review is only useful if you intend to use it as a stick to beat someone with - and hopefully you've got better processes in place to review performance including peer involvement in staff review meetings.

I'm not a huge fan of 'gated checkins' however it can be useful to hook into the tools that your source control provides such as using 'pull requests' as the instigators of a code review. These are especially useful if you are only able to complete a code review remotely. The pull request will become the shared notepad for your dialogue but should be part of a process that includes direct discussion. The key is to avoid moving through a commit history one file at a time (often in changed, or worse, alphabetical order) as this encourages a focus on the minutia.

No-one is above the law

If we treat code reviews as code conversations we're effectively removing seniority and absolute correctness from the equation. That is to say we enter them on equal footing both trying to make the code and our understanding of it better. If that's the case there's no reason why anyone within the team should not be subject to the code review process. A junior forcing a senior to explain something is just as valid and useful as a senior pointing out an error in a juniors work (and that isn't even allowing for the possibility that the junior, by questioning the dogma of a senior assists in a breakthrough for both).

But I work alone

The social aspect of code review's is a big plus. If you're working on different projects or 'in the zone' on your own code it can be easy to fall into going most of the day without really getting involved with your colleagues. Being asked to code review breaks that up a bit while avoiding the possibility of getting swept up in a crazy idea as you egg each other on during pair programming (by the way, I don't think pair programming is bad because of this but it is a plus point in the favour of post-coding reviews over sitting together throughout).

As an individual you might be able to rope in a friend from a different company, or a co-working space to help with code reviews. But if you can't you might try out sites like http://codereview.stackexchange.com/. Although in this case I suspect you may wish to be more targetted in the choice of code you submit - unlike within a team when there's no reason that every single line of code should not be reviewed.

So what are Code Reviews really about

Code review's are ostensiby about code quality but they have huge potential to increase team unity, better prepare team members for working across projects and assist in cross polinating skills. It does require effort and a move towards egoless programming but should be easily sellable within an organisation not least because of the surface argument of identifying defects.

As a manager you even get to play god a little bit... encourage team members who you notice don't interact much to get involved with reviews together, ensure that people who've been on individual projects for a long time are assigned to review something wildly different or even get a junior to review some of your code in their first week.

It's also the easiest way to encourage and endorse discussions about code and work projects without sounding like you're trying to get down with the kids "hey guys It's time for show and tell what you got goin' on".