Paper Cuts
Whenever I meet someone who is performing systematic code reviews, I am thrilled. Code reviews are close to my heart, and, believe me, meeting such a person is not a common event as one might think. So whenever I do meet such a person, I just want to know everything about how he handles his reviews.
Like any activity, code reviews has intrinsic aspects and technical aspects. In previous posts about code reviews (such as this one) the focus was on the essential aspect of code reviews: their goals and their spirit. But sometimes the technical aspects are substantial enough to turn code reviews into success or failure. How you read the code is one of those important technical aspects.
You see, when conducting a review many people read a printed version of the code. Although this issue looks insignificant at first, it has an immense impact on the effectiveness of the review.
The Non-Linear Nature of Code
Code reading is not linear. It can’t be. You cannot read code the same way you read a book or an article: line after line, paragraph after paragraph, page after page. Code is more complex than that. When you review code, you have to jump back and forth. You have to drill down into code entities to verify their behavior. You have to step out of code entities to verify their usage. Doing all that with a pile of papers is not only ineffective - for most people it is impossible.
The only proper way to analyze code in code reviews is using a tool that enables you to navigate the code freely. A common IDE might be sufficient for this purpose, but I usually prefer an even more advanced tool. Source Insight, for example, is a great tool for this purpose. Its syntax coloring, navigation options, code database, and sophisticated search options provide me full control over the code. That control enables me to find subtle problems hiding in the code - problems which would have been impossible to find otherwise.
Reading Code Effectively
You might say that this is merely a matter of taste or convenience. It’s not. When you read code from paper, you are usually limiting yourself to looking at one block of code at a time. You will certainly find some problems in the code, but most of these problems will be local. Subtler problems, such as problems in resource management, thread synchronization, or inconsistencies between the definition of an entity and its usage, are not local. There is practically no chance of finding such problems using linear reading. At least not with a reasonable effort.
Reading code from paper is like looking at an image one pixel at the time. You miss the context, the interactions between different elements in the image, and the big picture.
To make a real sense of an image you first look at it as a whole. You scan it with your eyes. Something catches your attention, so you concentrate at it and analyze its details. Then, you move back to the high-level resolution trying to understand how the piece you’ve just seen interacts with its context. From there you move to a different section of the picture, and so on. In order to analyze code effectively you should do just that.
Next time you’re going to perform a code review, don’t print the code. Try to find a more effective way to read the code and make a sense of it. Try to find a tool that doesn’t limit you to linear reading. A tool that allows you to navigate code freely, to control it, and to look at it from different perspectives. The quality of your review depends on it.
July 11th, 2006 at 4:42 am
I’ve actually found printing out code to be very helpful in deciphering old functional/procedural code snippets when having to understand the business rules built into them. They more often than not end up fairly linear in nature within the class/method anyway.
With the more object oriented code these days and all the programs in the web environment, I can completely agree with the notion of sticking to an IDE like eclipse that will allow you to follow workflow by clicking through the methods instead of working line for line.
July 11th, 2006 at 9:05 am
Hi Retrospector,
Basically, you are right. C-style code is more linear in nature than modern object oriented code. However a reviewer can benefit from using an IDE or another code reading tool when reviewing C-style code as well.
With an advanced tool you can easily spot dead code, for example. You can also easily locate variables declaration and usage. And of course you can always use bookmarks to navigate between important lines of code.
All these capabilities are impossible to achieve when reading the code from paper. The fact that code is linear does not necessarily suggest that reading it should be linear too. Especially not for reviewing purposes.
Lidor
July 13th, 2006 at 5:38 pm
I fully agree on the importance of code reviews. What I find particularly bad about reviews is that they usually happen so far down the cycle. It’s much more effective to do it when the code is at least fresh in someone’s mind.
One of the best experiences I ever had as a co-op student was spending weeks sitting in code reviews for with some very experienced coders. Osmosis is a great thing.
Much like how Microsoft Word has a mechanism for comments and change tracking from multiple users, it might be nice to have something similiar with an IDE.
Although, with good source control in place, it could be as simple as each person taking turns going over the code and adding comments with some metatags and then checking it in so the next person in the chain could review it, then having a round table at the end of it all.
July 13th, 2006 at 9:39 pm
Hi engtech,
You’re completely right. The sooner a review happen the better.
I found weekly code reviews to be the most effective. A week is relatively small interval which allows the code to still be fresh in the developer’s mind. At the same time the code written in a single week is quite manageable from the reviewer’s perspective. A shorter interval might also be good but there is a chance that this will break the development flow.
You’re also right about osmosis. I treat code and design reviews as a mentoring platform: a platform through which knowledge and experience is spread among developers.
I invite you to read about my approach at:
http://lucidquality.qualityaspect.com/Overview/
Thanks for the feedback,
Lidor
July 20th, 2006 at 1:26 pm
I do code reviews on my team’s code about once per deliverable. Sometimes I’ll review all the code, sometimes I’ll choose a subset of the code. I always work off paper - partially because it means I can take the code away and find a quiet place and work without distractions. Once per deliverable isn’t often enough to prevent re-work, but is something I can commit to. Ideally it would be more often, perhaps weekly as suggested above, but certainly a couple of times per month. I’m going to go away and think about how I can fit more code reviews in to our delivery cycles.
I currently review SQL stored procedures/functions as well as .Net/VB code. I usually review SQL against our coding standards as well as what I deem to be good practice.
For a .Net code review, I usually require that the code has been FXCop‘d - this cuts down on the search for un-used variables, as well as a certain amount of sloppiness and bad practices, leaving me to concentrate on overall design. I agree that this would be better served in the IDE rather than on paper.
The best code reviews are when we have a discussion about why a piece of code has been written in a certain way.
Great to read other people’s perspectives, thanks for sharing.
July 21st, 2006 at 1:52 am
Hi Jane,
Thanks for the feedback and for sharing your thoughts and experience.
I think the most important parts of your comments refers to having a discussion about why a certain piece of code was written in a certain way. When I conduct code and design reviews I have two goals. The first is improving the product under review. The second is enriching the experience of the developers who wrote the code using their own code as the learning material. An open discussion about the code is clearly the way to do this.
I think your approach is great. Keep doing what you’re doing. You’re clearly on the right track, and your team is very lucky to have you as their lead.
Lidor
July 25th, 2006 at 2:45 am
Interesting points, but I’d like to disagree a little bit. I’ve done my share of reading other people’s code and I often find it to be very helpful to print the code. I generally use colored pens and some extra sheets of paper to follow the code flow.
I do agree, however, that it gets harder and harder as modern software relies more on very heavy object oriented standard libraries. But the color pens and free form scribbling on paper is still very important to me, although I use code inspection tools as a complement.
July 28th, 2006 at 5:53 am
If you are like me who prefers keyboard over mouse, source review using ctags on Vim/Emacs (with Syntax coloring, even on Windows) is the best.
One can easily follow the C/C++ code - jumping back and forth to definitions, header files, macros, etc.