Effective Code Reviews
July 14, 2008 – 1:06 amAs our company has grown from a staff of a single engineer (yours truly) to over twenty, we have evolved our development practices as well.
Though our initial focus was on what I like to call the “Git ‘er done” development practice (not the same as Getting Things Done), as a software product company, we (and I) have learned painful lessons about writing code that is easy to maintain, extend and test. Part of these learnings has been to put a much greater emphasis on code quality, and by extension, code reviews.
Of course we are still a relatively small group, one that prides itself on flexibility and rapid turn-around-times, and so we attempt to only “Do what is needed”. This applies to everything, including code reviews. Because of this philosophy, I have tried to develop a stripped down code review process. It typically focuses on these five elements:
Code readability
Code readability means examining the code to ensure that it is well laid-out, reasonably well-designed, and easily understood by an outsider. This is critical for determining whether others can maintain it.
Exception Handling
Exception Handling is a focus, because, quite frankly, I have found that many developers do a poor job with it, and it leads to issues when we get to production. The frequency with which exceptions are swallowed or not logged with sufficient information for the developer to act is startling to me. I don’t believe it is particularly tricky either; it means asking oneself whether the calling routine is going to do anything meaningful with the exception. If the answer is no, I recommend logging the full stack trace and rethrowing it as a runtime exception. If yes, then, well, do it!
Commenting
Goes without saying, but without the hook of a code review, many developers do a poor job of commenting their code. The expectations we have on this are not enormous; our general advice is comment per method to describe what the method does, and then inside to add a comment for each control structure (loops, if-else if-else statements, etc).
Unit Test Review
Many developers struggle with writing good unit-tests, especially when it comes to isolating the class in question. Ensuring that unit-tests are properly written is probably one of the most critical reasons for our code reviews, and the area that we focus on most.
Environment Hygiene
This is an interesting one, and it has to do with the numbers of times I have the following experience:
Programmer X asks me for help. We sit down, take a look at his code. I suggest a change, he attempts to redeploy it. The redeploy involves a compilation (”And I just ignore those errors and warnings - I’m not sure what’s causing those”), some sort of manual deleting, file movement, and then an application server restart. By the time they are done, its been about five minutes, and watching them go through the process, it is hard to not think that they could make a mistake on one of the steps and not even have gotten the change pushed through(which can lead to even more insidious issues).
Environment issues are tedious, but they have to be addressed, and they should be, if not immediately, then soon. The goal should be a clean, DRY(don’t repeat yourself - don’t force yourself to have to make the same change in two places), fully automated process.
So these are the items we have come up with so far to focus on during our code reviews. They have been prioritized based on what I think as giving us the greatest return on the time we invest in them. What do you think? How do you do code reviews and what does your company emphasize?
-John
9 Responses to “Effective Code Reviews”
John: I arrived at this posting via DZONE. Nice post indeed! Yes, I agree with all of your major points in this post. The truly best code is code that can be reused for yet another purpose…so we don’t have to always reinvent the wheel, etc. Thanks for sharing.
All The Best, Keith Johnson (techwriter007 at DZONE)
By Keith Johnson on Jul 14, 2008
I agree with all of your points, but would add the following:
Architecture
This point could be considered one minor aspect of readability, but really has a greater impact on maintainability and scalability. I believe that a team of developers should be working together to arrive at and maintain a common architecture at a component level (since various components may have different architectural patterns and goals). Code reviews can be a great way to reinforce good architecture. If a change deviates too greatly from the agreed upon architecture, more work may be necessary.
Style (as applicable)
Some companies have strict coding styles and conventions. Code reviewers should watch for major deviations from documented guidelines. This overlaps slightly with readability, but has slightly different implications.
By Jeff Kinzer on Jul 14, 2008
Nice write up. I particularly enjoyed Environment Hygiene.
By Mike on Jul 14, 2008
> describe what the method does
Sometimes it might be more useful to describe WHY a method does a particular thing and WHY you wanna use it. I think the WHAT should be suggested with the method name. For example:
public List filterByName(String argName)
I think it’s pretty clear what it does.
By david Linsin on Jul 14, 2008
@Jeff: With regards to style, I agree that it is worthy of attention. It has tended to not be much of an issue, though, as we posted a standard eclipse style sheet some time ago, and (somewhat to my surprise, as initially some of the senior developers seemed very resistant) almost everyone has been pretty good about following it.
With regard to architecture, this one is a little trickier. Though in principle I would like to make this a key focus for evaluation, it has not turned out that way. That is not to say it is not discussed; in fact, I would say that one of the most enjoyable aspects of the process has been the opportunity it provides for myself and other senior engineers to bat around our own philosophical views on good design.
I think this is useful for all involved. For the more senior folks, it is a chance to learn and refine our own practices. For junior developers (who I do not expect to fully follow the conversation), it is a chance to be exposed to ways of thinking and problem-solving that, though they may not fully grasp their importance, will hopefully plant a seed that they can develop on.
However, I hesitate to include much in the way of evaluation or compelled revision based on what we see. I guess my hesitation is that, as mentioned, I don’t expect more junior engineers to understand all the points at stake, and forcing them to go back and re-write in line with my or someone else’s view of how their code should be factored seems unfair. If I was willing to dedicate all the time involved to help them do this, perhaps then I would, but typically I’m not.
Now there are of course exceptions to this rule. If someone chose to use, say, IBatis instead of Hibernate for their data access in an application that has consistently used hibernate, I would be pretty ticked. This sort of inconsistency is needless. Typically this sort of issue has not come up, but should it, I believe rewrites might be in order.
I would also add the architecture-wise, we do design reviews at the initiation of development. Usually these designs are laid out by our more senior staff, and so the end product is not much of a surprise.
I would though still end this with a question, as I have only come to this view of de-emphasizing design after a fair amount of consideration and thought, and that is: how does one incorporate design aspects in reviews with less-skilled staff without overwhelming them? I like my guidance to be concrete and discrete, and, frankly, my design guidance tends to often be far more abstract and philosophical. I guess if I could parse it down to something simple and easily followed I would be more likely to make it mandatory.
@david: Yep I agree, though my focus tends to be more on the how with commenting. If filterByName does some weird tricks (say retrieves everything and throws it into a hashmap, then uses the hashmap to filter - not sure why you would do this, but let’s pretend it was necessary) then a brief comment mentioning this would be appreciated. I hate comments like “Filters by name”. That’s up there with commenting simple getters and setters.
@keith and @mike: Thank you. I’m hoping to be much more active on this blog, and the feedback is appreciated.
By jkelvie on Jul 15, 2008
There’s no doubt about it, static testing of code in the form of formal reviews - no matter how simple the process - can have a dramatic effect on the quality of software.
You should see a cumulative effect, too - the word that comes to mind is ’synergy’!
By Brian Heys on Jul 16, 2008