personal web log written by izabeera and dryobates

code review

Reviewing big chunks of code

by dryobates

There are two main goals for making code review: making better code and making better programmers. Not only reviewed developer can learn, but reviewer also. I will share my experiences as a reviewer.

Recently I was doing code review of a code created during last 4 months. That is quite long release period in my company. Until now I mostly done reviews of code that was created not longer then 2 weeks (and it already was a painful experience). That 4 months long developed code gave me an unique opportunity to face with doing code review on really big chunk of code.

First problem I was faced with was that redmine [1] didn't allow me seeing the whole diff because of it's size. I really only need to see one file diff at a time so it would be sufficient to open in redmine only one file. I wrote small script that generated links to redmine diffs for every file:

#! /bin/sh

die() {
    echo >&2 "$@"
    exit 1
}

[ "$#" -eq 3 ] || die "usage: ,cr <domain> <from rev> <to rev>"
REPO=$1
REV_FROM=`hg id -r $2 -i`
REV_TO=`hg id -r $3 -i`
for f in `hg st --rev ${REV_FROM}:${REV_TO} -n | grep -v 'migrations\|.css\|.png\|.jpg'`
do
    echo "https://<repository>/projects/${REPO}/repository/diff/$f?rev=${REV_TO}&rev_to=${REV_FROM}"
done

Now I could review file by file creating review comments. We use redmine code review plugin [2] Although it's not sophisticated code review tool (no option for creating pull requests, analysis coder - reviewer pairs etc.) but for now is sufficient for us. The main benefit of it is integration with our current ticket tracking system.

Because of the size of material I had to split review to 4 days and still had a headache while reviewing. Now I'm absolutely sure that code reviews should be plotted inside developer work. I could give a faster feedback for developer and easier reviews for reviewer. We have to yet figure out how to implement it in our workflow. In my opinion pair programming would be ideal type of code review, but I'm afraid it's a long time to convince management of it's benefits.

Code review of python code was quite easy for me as I code in python for a long time. I had worse time with django templates where is much more noise then code.

I done code review trying to follow rules, that many reviewers highlights:

  1. First I tried to find out if changes in general have sense.
  2. Then I have scan line by line with a focus on bugs and bad design.
  3. Last phase was looking for deviations from style.

Thanks to that order I didn't have to create a lot of comments on small issues if the whole code should be rewriteen.

Because code was written by young developer I had opportunity to see many common errors and some really unique.

The hardest part of that code review was creating review tickets. And I don't mean technically creating tickets. The hard part is formulating description. How to say someone that he can do something better in a manner that he won't feel being criticized. Many reviewers suggest using questions but it is sometimes a way easier to simply paste a chunk of code and say "that would be better".

That is a part that I have to train more.

[1]Redmine http://redmine.org
[2]Code Review Plugin http://www.redmine.org/plugins/redmine_code_review
dryobates
dryobates
Jakub Stolarski. Software engineer. I work professionally as programmer since 2005. Speeding up software development with Test Driven Development, task automation and optimization for performance are things that focus my mind from my early career up to now. If you ask me for my religion: Python, Vim and FreeBSD are my trinity ;) Email: jakub@stolarscy.com

Archive

Tag cloud