Posterous theme by Cory Watilo

Code reviews: turn that frown upside down

About a year ago, I started working in an environment where code reviews or other forms of feedback weren't considered important and weren't a regular practice.

After several months of infrequent code reviews, what are the biggest issues with introducing, maintaining and improving a code review process?

Aggressive Reviewers

This is perhaps the most common thing that comes up during code reviews. Of course, review is there for everyone's benefit; the codebase, the team and each one individually, and not to attack people personally or make them feel inadequate. If they stop putting their code up for review, either bad code will make its way into your product, or only the most trivial of code will be submitted, leading everyone to think that everything's good when in fact it isn't.

The fact of the matter is that we're all human, and we make mistakes. Often those are the result of accumulated pressure and requirements, incomplete problem knowledge or conflicting specifications, etc. We've all been there, and we've all looked at code we've written years ago and thought "Gawd, this sucks!".

Dealing with over-aggressive reviewers is easy. Taking one aside and criticizing their approach will resolve the issue quickly - most of the time people don't even realize how they're coming across when in fact it isn't their intention to appear aggressive.

"Victim" mentality

On the other hand is the code "owner" who considers any kind of critique to be an attack either on their skills or their person. Their attitude is usually defensive against any comments, instead of seeing a review as a learning opportunity. That isn't to say that all review comments are positive, of course, but one should not outright dismiss them.

"Protecting" your code doesn't help in any way; the resulting situation is bad code making its way into the codebase, faults remaining hidden, etc. No good can come out of no review.

Negativity

There is nothing wrong with looking at code and not having a bad thing to say! Nothing wrong with just saying "That looks good"!

Lack of an outcome

Code reviews are supposed to incite arguments and disagreements, because that's the best way of reaching useful conclusions. But to reach a conclusion, you need to reach a decision.

At some point there needs to be recognized who has the final say in things; a manager, a senior, someone explicitly responsible for code quality, whatever. Remember that you can't please everyone, and unfortunately that's true of code reviews as well.

Review is a constant process

Not all review comments have to be acted upon. Not all "Hey, this is cool! We should use this more often!" warranties a policy of reusing Good Code™, and not all negative comments have to be satisfied. However, if devs are seeing their comments, review after review, being ignored or not acknowledged, it can be terribly demoralising and poisonous for your team.

Being ignored leads to two things: less participation, less reaction. It devolves into a chain reaction that will eventually get you nothing done, and harm the dynamic of your group more than code review can ever help you. If somebody takes the initiative of "bumping" review comments for action or re-opening review issues for discussion a problem might get resolved soon enough.

Lack of focus

It's a common sight that people don't know what to review. You can recognize these occassions when the discussion starts to devolve into conformance to coding principles or indentation levels.

I've found a gem that works wonders here. I ask people what do they want to focus their time on during the review. Suggesting areas to focus gives people the initiative they might lack; if this is able to shift the conversation from "tabs vs spaces" to seven people looking hard over a critical piece of your application logic, you have succeeded. Next time, people will know what to look for and be more alert. System knowledge increases and they have more of a clue on what's important for your code, and what isn't and should be left alone.

Unrealistic expectations

Development has no silver bullets. No process, technique, or practice will take you to communication and software nirvana, and there is no way to stop all bugs at code review or prevent your code from "rotting".

If you start believing code review (or pair programming, testing, verification) will magically cause bugs to reveal themselves before shipping, or that everyone on your team will suddenly rise to the level of your highest achievers, YOU WILL FAIL.

Accept that code review has an overall positive effect on your work. Your team will get better and the quality of your product will improve, if you save your developers the disappointment that wishful thinking like above entails.

I'd really like it if I could put some ways we've reviewed code up here and describe where we went wrong and what benefit we had, but time's running out. Maybe some other time. :-)

| Viewed
times | Favorited 0 times
Filed under:      

2 Comments

Oct 04, 2011
hakmem said...
The benefits of code reviews are also eloquently presented in Tom DeMarco's novel "The Deadline", together with budget saving arguments.

Cool post!

Oct 11, 2011
Code review AND budget savings? That book must feel like the Bible. :-)

Leave a comment...