Home > General Software Development, Pair Programming, TDD, Test Driven Development > Code Review – Standing on the shoulders of smart people

Code Review – Standing on the shoulders of smart people

A veryintelligent person once said:
“If I have seen further it is by standing onthe shoulders of Giants.”
The person in question, is of course none other than Sir Isaac Newton. He was able to go further with his discoveries because others had solved some of the details already and provided a layer of abstraction for him to improve upon.

When doing a code review,I often encounter a little defensiveness. Since we practice pair programming almost exclusively, this code review usually happens when reviewing code (as a pair) to make a change for a new feature or bug fix. Often the code was not written by the pair reviewing it so it becomes easy to get offensive instead of defensive.Either way, thisworks against the purpose of the code review and can be solved by thinking aboutthe code reviewin a different way…

The pair (or person) who wrote the original code was dealing with different complexities and challenges – they solved these problems with the code in question. Our job when reviewing is not to solve these same challenges but rather to look at *theirsolution* and decide how it could be improved. In this light, it is very unfair to compare our improved solution with theirs since we had a very different task. This is a subtle but very important distinction. Remember this when doing code reviews and it should help you to move beyond the personalities and egos involved (well, maybe :)).

How do *you* solve the human challenges of a code review?

Jonathan Cogley is the CEO and founder of Thycotic Software, a .NET consulting company and ISV in Washington DC. Our product,Secret Server, is a secure web-based solution for teams to secure their passwords. Where do you keep your passwords or do you use the same one everywhere (snicker)?

  1. Ted_Graham
    March 19, 2007 at 2:17 pm

    Developers are often over-confident, which leads to rewriting of code they don’t understand under the guise of refactoring. Code review should not be an excuse to change working code, but should be an attempt to improve the code.

  2. Jeff Schoolcraft
    March 19, 2007 at 10:53 pm

    Jonathan, I a bit surprised. One of the side effects of pair programming is “Collective Code Ownership”. CCO should go a long way towards creating an ego-less environment.

    I’m not immune from human emotion, I too have been caught up with a little pride or hubris in particular bits of code, but I’m guessing you’re a lot better off through pairing than a formal, after the fact code review of a code cowboy.

  3. Jonathan Cogley
    March 20, 2007 at 12:06 am

    Jeff,

    Interesting that you mention CCO since it is that which drives us to critique the code. We usually notice the code because it stands out from the norm (bad naming or unnecessary complexity or a weird pattern). Once this is identified then if it is especially weird, we will ‘blame’ it to see the commit messages + the evolution of said code, time of the commit :). Then we fix it so that it doesn’t stand out from the norm anymore.

    Thanks for posting – the team is looking forward to your Code Camp!

  4. Jason Kealey
    March 20, 2007 at 12:44 pm

    Cenqua is about to release v1.0 of their Crucible code review product. I’ve tested the product and it is really nice.

    http://www.cenqua.com/crucible

  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: