Options For Distributed Code Reviews

0 comments suggest edit

Screen Showing Code The great thing about being involved in a couple of open source projects is that they provide great opportunities to learn and to teach, especially through the vehicle of code reviews.

To enable this within the Subtext Project, I recently set up commit emails within the CVS repository. This makes it possible for more people to review changes to the code. However, this brings up a bit of a quandary. How should we go about conducting the reviews? We don’t have any policy in place.

Ideally I would love to have regularly scheduled code reviews in which a small group of developers gather around the screen and review the code at the same time. But as several of our team are in Europe, that is just not feasible. At least not so soon after the code is checked in. I think it would be helpful to have more immediate feedback.

There are two ways to handle this that I will put forth in the hopes of soliciting feedback.

Make Suggestions In Comments

One approach proposed by Steve Harman is an approach he uses at work. When reviewing code and finding something that might need to be changed, the reviewer puts a comment like so:

//REVIEW: haacked - Ummm... Hey, this violates an FxCop rule. Perhaps you should do FOO instead.{.example}

What I like about this approach is that it is deferential. You can shoot off an email to the person who wrote the code and say something along the lines of …

Hey, the code you checked in looks good. However, I noticed a few things you might want to look at. I put the appropriate comments in the code.

This approach allows the original author the chance to learn from the reviewer comments as well as to defend or elaborate upon the code if he or she feels that the code is indeed correct. Should the original author not have time to change it, the reviewer can return to change it.

Change the Code and Notify

One drawback to the previous approach is that it takes two people to fix code. One thing you’ll find when participating in an open source project is that people are REALLY BUSY. Sometimes a contributor will contribute a piece of code, and then get slammed at work and home, only to return in three months time to contribute again. That’s a long time to wait for code to get corrected.

An alternate approach is one borrowed from certain agile methodologies. Practices such as Extreme Programming promote the concept of “Collective Ownership”. This is an important principle to apply to open source projects, as the code belongs to everyone. Likewise, everyone should feel free to contribute to any piece of code in a project, assuming they use good judgement and understand the code.

In this approach, one would just fix the code and in the source control checkin comments, explain why the change was made. Additionally the reviewer could send the author a quick email mentioning that he made some changes and to keep an eye out on the commit emails.

The benefit of this approach is that the code gets improved immediately, while still preserving the learning opportunity for the original author. This approach also addresses the fact that any code you change in codebase most like at one point or another was written by someone else. Sometimes a reviewer might be looking at code long after it was committed. The reasonable expectation here is that the reviewer should feel free to modify the code, rather than spending time figuring out who wrote it and making the review comments.

Something In Between?

Perhaps there is an even better option that sits somewhere between these two options. What do you think? I would love to hear from you in my comments.

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.



7 responses

  1. Avatar for Simone
    Simone February 2nd, 2006

    MHO is that the people that finds the problem should try and fix it... and eventually write an email to the original creator of the code.

  2. Avatar for Steve Harman
    Steve Harman February 2nd, 2006

    Well, since Phil went and mentioned that it was my idea, I suppose now I have to defend it. :)

    I do understand [and agree] that in an Open Source application, time is even more critical than in a "Work Project." As a result, the time lapse that it takes for a reviewer to look at the code, make comments, and then have the original author make any changes is too great. People are busy... believe me, I understand that! But who's to say that the reviewer will have the time to write up an email? What if they are reviewing several files... they could have dozens of fixes to make. Are they necessarily going to remember where and what all of those changes were? Yes, they could do diffs in source control, but again, do they have time? Also, an email out the the original author doesn't help anyone else to learn anything. I'm a big believer in learning not only from my mistakes, but also from those of others.

    I think that Haacked hit the nail on the head... what we really need is a hybrid approach. So, I'll simply re-iterate his request for ideas from you, the community.

  3. Avatar for Gurkan Yeniceri
    Gurkan Yeniceri February 3rd, 2006

    I think that using the VS Task List is a great idea. It is the first place that I am looking for any changes, if the code is properly commented and token list is up to date.

    In Open Source projects though, collective ownership is a good practice if you are sure that the fix your are applying is actually fixing the code. If you are not sure you can always use REVIEW: task. You still need to properly comment out what you did with a date signature in the code.

    So, fix if you can, put your initials, a date and a small description explaining why as a comment. Also I don't think it is necessary to create a bug record and attach your changes to this bug. The best thing would be to link the change to the bug as I explained here http://www.analystdeveloper.com/blogs/gurkaneng/archive/2005/09/20/1465.aspx

    using bugtraq properties. It is possible with Subversion, WebSVN and TortoiseSVN

  4. Avatar for Haacked
    Haacked February 3rd, 2006

    Well if the team subscribes to the commit emails, then everybody can learn from changes made in the code, whether we add the REVIEW tag or just fix the code.

    In general, I'd like to keep out unnecessary comments in the code. For example, comments in the code should only describe "why" and not "what". So initials and a date do not belong in the code. Instead, that should be in the check-in comments.

    Not every review change is fixing a bug. Sometimes it's just changing code to conform to the project's established style. Sometimes it is changing code to conform to FxCop rules.

    For example, we should check for empty strings by comparing the length to zero. If someone puts code that compares a string == "", I'll just change it to the FxCop rule. This is a style guide for Subtext.

    Perhaps the best rule is to just change it for style changes and minor bugs, but add a review tag for a major bug. And every developer uses their judgment to distinguish.

  5. Avatar for Paul
    Paul February 5th, 2006

    Here we follow a lot of the XP practices, and I tend to just make changes. Especially when I find instances of people catching Exception and rethrowing (rather than just using the finally blocl), or when people comment out large blocks of code etc. I guess I'm relatively intolerant.

    However, I'd say that it's important to make sure that you have good test coverage -- I read a paper over the weekend about a team at ThoughtWorks on a project, with a developer who committed some changes late on Friday that introduced a deadlock scenario to the system!

    I'd be inclined to say that the XP way is to take total team ownership of the code -- fix where you see fit, improve/refactor where you can. Hopefully people will see an evolving design and follow the path that's created.

  6. Avatar for luke
    luke March 31st, 2007

    A slight twist on the tactic could be to encourage authors of complex and/or critical code to add their own review-request comments. It REQUIRES no time on reviewer(s), and it adds only a couple extra seconds for the author (who is already spending time writing the code anyway).
    Then other engineers see a change notification email with the following:
    // ****REVIEW****
    // Please check to see if the following can be simplified
    They can look it over, and either simplify/fix the code themselves, or send an email, or whatever action time allows.
    This does rely on having "humble" authors and attentive groups, but it seems those are the best engineers in projects anyway.

  7. Avatar for Haacked
    Haacked March 31st, 2007

    Yeah, I like that idea! It's very similar to Steve's idea in the body of this post.
    He uses the following syntax:
    //REVIEW: Please check if this can be simplified
    One reason to use the "Slash Slash Review" syntax is that it uses the same syntax as the other TODO items that VS.NET 2005 recognizes and adds to your list such as //TODO and //KLUDGE.