Code reviews done "right"

This is the second post in a series of posts related to 10 things I don't believe in.

• Part 1

"I don't believe in code conventions" - hmm... Did I really write that? How can reviewing code be bad? It's not, actually. Code reviews can be beneficial if they are done right because to have someone review code you have written is one of the top 3 things you can do as a software developer to improve your skills. It's just the way things work - you can be a winner in life, in love and in any Xbox game by not repeating your mistakes more than once - and you will be a pitiful looser with no friends and no money if you have one success and spend the rest of your life doing whatever made you a success in the past over and over again.

When I think "code review" I think:

• Get a timeslot in everyone's calendar and book a meeting for i.e. half an hour or an hour
• Once at the meeting: Get everyone lined up in front of a projector
• Pick some code from the project you're working on
• Have the person who wrote it to walk you through
• Feedback

95% of all the formal code reviews I have ever attended followed that prescription - and it never really worked out the way it was intended for the following reasons:

• Get a timeslot in everyone's calendar and book a meeting for i.e. half an hour or an hour
• Developers hate meetings. It takes time from their coding, quite literally. Developers don't like to be interrupted unless they feel an urgency to attend the meeting - and code reviews aren't percieved as something you'll die of if you don't get your weekly dosis.
• Get everyony lined up in front of a projector
• The more formal the approach, the merrier. There might even be some sort of agenda: "Class X: 10.00 A.M, Class Y and Z, 10.20 A.M. T-SQL changes 10.40 to 11.00 A.M"... Let's see some code, right?
• Pick some code from the code you're working on
• Is everyone working on the same project? If yes that's fine - proceed. If no, you'll lose every single developer who hasn't been actively committing to the code you've picked. People just don't feel committed to concentrate on understanding Javascript if they regard themselves as being primarily Database guys. And the clientside guys and gals don't have much input to a discussion about classes not implementing IDisposable in a correct manner.
• Have the person who wrote it walk you through
• Being up on the stage in the spotlight takes some balls to everyone who isn't used to being looked at - you're in a very vulnerable position up there. Also having to communicate some crappy code you wrote in front of perhaps some senior developers who you know have at least a dozen better ways to implement a variation of the combined Visitor / Strategy pattern you've been working on can be frightening.
• Feedback
• If basic trust and respect between the parties attending the code review aren't there code reviews can actually do more damage to team morale than anything else because creative people (like developers) tend to block their minds when being critizied because they put so much of themselves into their work. Even if a developer has got the arguments to prove his way of doing things he might not be able to communicate well because of the spotlight and the crowd of people staring at him.
• If feedback to code reviews aren't backed by i.e. code conventions or architectual documentations in terms of "this is how we do things here when we deal with datetime formatting" they will mostly be based on personal bias towards a solution you like better - not necessarily the right solution for the problem at hand. That's the way it works in real life if there are no code quality measures beyond the team that the code should live up to.

Only very mature teams can handle a code review which includes a projector, I think. And why, why, why make things so complicated? Why call everyone in and disturb everyone because "code reviews are important"? When I want my code reviewed I do this:

• Write some code
• Ask anyone of my collegues to look it through
• He or she moves to my desk and we sit down together and go through things together
• We might refactor while pairprogramming
• Put my taskcard on the "To be verified" column on our sprint board

This informal approach has a number of advantages: It doesn't feel like a code review. You don't get to attend meetings which you feel eats away your coding-time. Nobody is put up on a stage and asked to speak loud and clear because there are 6 people sweating and a projector humming in the background. Feedback happens informally and instant - and you feel safer walking through the code because it is fresh in your mind (you just wrote it, remember?). Feedback is also more valuable because it happens 1-1 in an atmosphere of "how do I solve this problem the right way". You're in problem-solving mode while sitting at your desk - chances are that you're not if you're being interrupted on-stage while trying to remember why class X implements interface Foo instead of the more general interface Bar.

Martin Fowler once wrote that If it hurts do it more often. If code reviews don't feel right you're doing it wrong - and probably you should begin by loosen up the tie and try a less formal approach because informal approaches always works the best while dealing with developers.

Until next time...

Regards K.