Code Review - Standing on the shoulders of smart people

Published Sunday, March 18, 2007 9:47 PM

A very intelligent person once said:
"If I have seen further it is by standing on the 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, this works against the purpose of the code review and can be solved by thinking about the code review in 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 *their solution* 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)?

Comments

# Ted_Graham said on Monday, March 19, 2007 10:17 AM

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.

# Jeff Schoolcraft said on Monday, March 19, 2007 6: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.

# Jonathan Cogley said on Monday, March 19, 2007 8:06 PM

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!

This Blog

Syndication