Sunday, April 21, 2013

Code Review Goals

I did a huge number of code review sessions in the passed years, gathered a lot of experience from my mistakes and I thought now it's a good time to share my findings with the world!

Finding 1:  Code reviews must be taught and learned

It doesn't seem obvious that a simple code review needs to be learned. If you have an intermediate or senior developer in your team, he or she should be capable without too much effort to do a "proper" code review, right? 

Well, from my experience this is not always true! Becoming a good reviewer like becoming a good coder takes time and practice. Although good engineers have a good sense of how a good code should look like, they are not always comfortable with the review process itself, it's underlying goals and benefits, approaches, etc. Same thing applies also for the ones that have their code reviewed.

For one of the projects in my current company I started to teach people how to do reviews by themselves and the results were simply outstanding: from a team of people highly reticent to have their code reviewed, the team arrived at a point where reviews were asked remotely by people working from home being in sick leaves, reviews were ranked as "properly made, with serious benefits, with a lot of discovered bugs" in team's retrospectives.

How did I do it? I invited as a first step in each review an additional developer to assist me, than in time I became the assistant and afterwards I didn't have to participate at all.
After a while everybody knew what they needed to do. I must admit I had my share of good reviews after that moment, a lot of bugs were prevented also in my code by my pear reviewers.

Finding 2: Review goals need to be defined and agreed

When trying to teach people how a good review should be performed, discovered that problems exist in understanding the purpose of various review activities. The easiest way to overcome this problem was to create a shared document that contained the goals of the various code review activities.

The goals and underlying review tasks looked like bellow:

Review Code

Goal: Reviewer must gain understanding on the reviewed part and be able to continue working on it if necessary

Reviewer must know what the code should do.
Reviewer must understand all changed code.

Goal: Reviewer must make sure that the code can be easily maintained

Reviewer should focus on naming and spotting bugs.
Check changes using "show differences" option

Goal: Reviewer must make sure that new changes are well integrated with the existing code.

Have a look over the entire class (maybe some refactoring can be done)

Review Tests

Goal of test review: Tests are more important than the code itself, they are our safety net!

Goal: Reviewer must make sure that tests will avoid bugs

Check that Not Only “Happy path” is covered

Goal: Reviewer must make sure that tests will help preventing "maintenance hell"

Check if integration tests can be replaced by unitary tests

Finding 3: Face to face reviews are priceless

Although there are a lot of tools that can be used for reviews (and used myself for quite a while ReviewBoard), discovered that at least in the early stages of project face to face reviews are priceless! 

When a team is forming, when the team members don't share a common language and the same set of values, the face to face reviews could become a real benefit. 

In these early stages it doesn't even matter if a review session discovers a bug, or not. What's most important here is that the team members have an extra chance to share their visions and set of values!

I know that you can loose some benefits when doing face to face reviews without using any review tool - collaborative reviews are no longer possible, reviewer has not enough time to try thinking out of the box, etc. But nevertheless, I will always opt for "face to face" if I'll have a choice! 

The real benefits

The real code review benefits? The shared language between developers, seeing my team growing, seeing that there is no difference in terms of quality and standards between our code and existing famous open source code, and so on!

No comments: