Constantly looking at commmits in SVN. Thats what I tried first. It didn’t work at all.
I found it rather hard to distinguish trivial changes like changing the name of a class (which causes lots of changed files) from important changes, like implementation of some algorithm or feature. So I spend ages looking at files, comparing them with their history just to find out nothing interesting changed.
Also it wasn’t clear which commits where considered complete features and which where just parts that where still work in progress. Even if I could identify commits belonging together I couldn’t combine them in a meaningfull way to consider them in a single code review. So I switched to a very manual process.
Coworkers would send me mails with references to code they want a second opinion about. This worked great because it hat two effects: First I could concentrate on code that already got identified as ‘interesting’ in some way. Secondly and possibly more important: Since the author of the code sent it in the feedback was much easier to accept then when I just reviewed code that I concidered in need of some reviewing.
The next challenge was to find a place to put the actual comments. I started with installing all kinds of tools in my IDE. Some of which asked for a conciderable amount of setup and configuration. But appart from eating away CPU cycles they didn’t really do anything for me.
I also don’t want my comments in the middle of the production code, committed to version control. Maybe it is just me but I don’t like putting stuff in production code that isn’t intended to end up in production. Thats why we have separate folders for tests, right?
So I actually ended up printing all the code for the review on paper (a thin white material, looking somewhat similar to a computer screen but much thinner). Paper is great for code reviews because you can draw arrows all over the place, color stuff and in general be very flexible in what you do. You can also spread out lots of pieces of paper on a normal desktop, which doesn’t work when you have only the equivalent of mobile phone display as a monitor.
By now I switched projects. The challenges are a little different but the tool is the same: code reviews. While in the previous project code reviews where mostly about finding solutions for problematic code, where developers knew something was wrong, but couldn’t tell how to fix it. This time we do code reviews as part of the definition of done. So it is much more about finding problems in the code and for me as the new guy in the project to learn about the code base.
We also have a different version control system: Git. And that is a big difference for me and my code reviews. I had to learn more about Git then I ever learned about SVN in order to just use it. But this learning showed me some awesome ways to use it for code reviews which are beyond what at least I was able to do with SVN.
When a task is finished by a coworker I run the following script to obtain a list of files that got touched by the developer for the given task:
git log --all-match --author="" --grep="" --name-only --format=format: HEAD | sort | uniq
The task tag is an identifier we use in the commit comments to identify all commits related to a given task. With --name-only and the empty format I get the file names only (no diffs) and also no headers (like author name, comments and so on) except for some blank lines. The sort | uniq sorts the list of files and removes duplicates, i.e. files touched in multiple commits.
I then proceed reviewing each file one by one. Using the ‘Show Annotation’ feature of my IDE I can easily identify the changes made lately and distinguish them from old code. I then make all my comments right inside the code with a “REVIEW” tag, so they are easy to search for. I do this now, because with git it is a no brainer to commit these comments in a fresh branch, so I have them version controlled but not in the production code, just as I like it. It also allows easy checking later if the issues found where actually fixed.
While this all sounds like small stuff and actually it is, it helped me with code reviews tremendously. Right now I’m thinking about trying Mylyn once more and find a way to convert the list of files to a Mylyn task context … we’ll see.
Reference: My Code Reviews – the Technical Process from our NCG partner Jens Schauder at the Schauderhaft blog.
