Derik Whittaker

Syndication

News


Images in this post missing? We recently lost them in a site migration. We're working to restore these as you read this. Should you need an image in an emergency, please contact us at imagehelp@codebetter.com
Code Reviews: How to be successful

Code reviews are something that not all of us like or do, but they can be very valuable.  I thought I would express some of my thoughts/opinions about the benefits and pit falls of performing code reviews.

Benefits of Code Reviews
Performing regular and consistent code can have a great impact on your project.  Some of the benefits can be:

  • Keeps code clean, can help enforce coding standards
    One of the great benefits of a code review it to allow the code base as a whole to maintain standards.  Along with maintaining standards, knowing your code is going to be reviewed will undoubtedly encourage the coder to write better/cleaner code.  Maybe they means adding a few comments where they would not have otherwise.  Maybe it means they will rethink their logic in order to make it cleaner, more readable. 

    All of this makes the code base better for future maintenance/expendability.

  • Forces/enables outside options on your implementation
    Another positive that come from code reviews is that it will allow other team members to possibly come up with alternative ways to solve your coding problem.  This can be useful because maybe there is a simpler way to solve the problem and you did not see it.  As we know, Simpler is Better..

  • Helps spread knowledge on code
    By having other people look over your code as well as having the intent explained to them, you are spreading the knowledge of your code.  This knowledge may only be in terms of intent, but the more people who understand the code base as a whole, the better.  You could also achieve this result with paring, but that is not the topic of this post

Possible Pitfalls of Code Reviews
Unfortunately there are a few pitfalls to code reviews.  Although in my opinion these are small, and most definitely can be avoided, you do have to give them thought.  Some of these pitfalls are:

  • Feelings get hurt
    The first rules to performing a code review is to Critique the Code, not the Coder.  NEVER, EVER, EVER attack the coder, only critique (not slam) the code.  If for some reason the developer of the code feels they are under attack they will go into defense mode and start defending their code from the wrong angle.  

    If this happens they will not hear ANY of the positives about the code, only the negative.  At this point all is lost, better off stopping the review and starting over later. 

  • Nothing of value comes out of the review
    If during the code review change requests are made, make sure they are acted upon.  If these requests are never acted upon and the pattern of this starts, the code review process is simply a waste of time.  The developer will learn that they don't 'have' to make changes and they will not. 

    Make sure to perform the follow up later by just looking for the changes, or actually start off the next code review by revisiting the old code to ensure the changes were made.

  • Too much time is spent on the review
    Code reviews should be short and sweet, 30 minutes tops.  If your review extends past 30 minutes, you are probably reviewing too much code. 

    By extending the review you will at some point start to lose focus.  When this happens the value of the code review starts to diminish.

Conducting Code Reviews
In order to perform quality code review there needs to Rules of the Game (hmm, sounds like the next post idea).  Without these rules, the review could get personal and this will not accomplish anything. 

  • Performed on a regular basis
    Code reviews need to be conducted on a regular basis.  This can be weekly, once a month, once a quarter, what ever, just so that they are regular. 

  • Keep the group small
    Keep the group performing the review small.  Maybe 2-3 'outside' people and the author(s) of the code being reviewed.  This will allow for the meeting to be shorter and more effective.  A large group will just cause process to drag on. 

    With keeping the group small, you can also create consistency with who is performing the review.  This consistency will lead to better results.

  • Know what you are looking form
    Before you start the review, communicate the what is being looked for and what is be expected during the review.  This will allow everyone to be on the same page and allow for better results.  Besides, open communication can go a LONG way.

  • Consistency
    Every code review should be as consistent as possible.  Make sure that the rules don't change from one review to the next.  Also make sure that what you are looking for does not change.  If in one review you are looking only at logic and disregarding coding standards, the developer will come to expect that next time.  When the next time comes around and you don't look at logic, only standards the developer will be confused and possible upset.

These are just some of my thoughts on Code Reviews, would love to hear yours...

 

Till next time


Posted 06-11-2007 9:08 AM by Derik Whittaker
Filed under: ,

[Advertisement]

Comments

Louis Haskett wrote re: Code Reviews: How to be successful
on 06-11-2007 3:09 PM

It’s important to note that doing a code review without any kind of design review before the coder starts coding is likely a waste of time.

If a guy goes and codes something for two week and then you have a code review, and in the review you say "i dont like how you’ve done it", well, it’s a little late in my opinion.

My main point is, doing a code review without understanding / approving of the design of the code is bad.  The code review will end up devolving into "you forgot comments here", "you used the wrong parameter naming convention here", etc.  Those are valid points for a code review, but they don’t really get to the heart of what you’re trying to accomplish with a code review.

Derik Whittaker wrote re: Code Reviews: How to be successful
on 06-11-2007 3:47 PM

Lou,

That is an excellent observation.  I actually was going to touch on that topic in my next post about which is about the rules of the game for conducting code reviews.  I think I may steal your quote for that post if you don’t mind.

I do agree with you 100% though, having knowledge of the intent of the code prior to reviewing it is very important.

Louis Haskett wrote re: Code Reviews: How to be successful
on 06-11-2007 5:14 PM

All in all i think your pretty on target.  

If nothing else, code reviews mean some %@^$# is going to get caught if they do something like, say, put 16 panels on a single windows form, one behind the other, and then write code to show/hide them based on business rules.

Not that someone would do something like that...

Louis Haskett wrote re: Code Reviews: How to be successful
on 06-11-2007 5:21 PM

And if they did, they wouldn't write such a form with 5100+ lines of code in it.  

Because if they did, and there were code reviews, the developer would be laughed out of the office.

Heck.  I'm.  laughing.  right.  now.

Derik Whittaker wrote re: Code Reviews: How to be successful
on 06-11-2007 5:37 PM

Lou,

Something tells me you ran into some really, really, really stinky code today.... :)

Louis Haskett wrote re: Code Reviews: How to be successful
on 06-11-2007 5:53 PM

So hopefully my little campfire horror story helps drive home the point:

Do Code Reviews!  Or the CodeBoogieMan's gonna getcha!

Louis Haskett wrote re: Code Reviews: How to be successful
on 06-11-2007 6:08 PM

A useful way to "prep" for code reviews is for the reviewers to look over the code before hand and add "//TODO" tags where they see issues.  Then, during the code review, the developers can discuss each to-do and either keep it, add some more comments to the to-do, or delete it.  This way you don't have to leave your review with a bunch of hand written notes.  The changes you need to make are already tagged in your code!

I like doing to-do tagging like this: “// TODO LHH 2007/06/11 – just do it” so that I can easily sort my personal to-do’s in visual studio’s task list, and at a glance see how stale they have gotten.

About The CodeBetter.Com Blog Network
CodeBetter.Com FAQ

Our Mission

Advertisers should contact Brendan

Subscribe
Google Reader or Homepage

del.icio.us CodeBetter.com Latest Items
Add to My Yahoo!
Subscribe with Bloglines
Subscribe in NewsGator Online
Subscribe with myFeedster
Add to My AOL
Furl CodeBetter.com Latest Items
Subscribe in Rojo

Member Projects
DimeCasts.Net - Derik Whittaker

Friends of Devlicio.us
Red-Gate Tools For SQL and .NET

NDepend

SlickEdit
 
SmartInspect .NET Logging
NGEDIT: ViEmu and Codekana
LiteAccounting.Com
DevExpress
Fixx
NHibernate Profiler
Unfuddle
Balsamiq Mockups
Scrumy
JetBrains - ReSharper
Umbraco
NServiceBus
RavenDb
Web Sequence Diagrams
Ducksboard<-- NEW Friend!

 



Site Copyright © 2007 CodeBetter.Com
Content Copyright Individual Bloggers

 

Community Server (Commercial Edition)