Philip Beadle's Blog

Today we were discussing the merits of code reviews and Rick threw this is in to stir the pot.

 I am going to play the devil’s advocate here: Don’t shoot the messenger J
This is me playing a PM/Primary stake holder in a project you are working on…

What is the problem a  code review (by 2 others) meant to solve exactly?

1. Do your coders produce code that does not build?
   1. Nightly builds and admonishment for bad practice will solve. 
   2. Code review will not and is in fact a waste of time.
2. Do your coders produce code that does not work in the first place?
   1. And you would answer: We test our code before it gets checked in. 
   2. A code review is no replacement of unit testing.
3. Did your team build something in manner inconsistent with our systems.  Did they introduce dependencies that we had not planned on.  Did they use libraries, third party tools, etc not compatible with our environment?
   1. This is what a design doc is supposed to catch: Before they actually spend the time to code it and then find out after the fact. Stake holders love it when you do thisJ
4. Did they code not following a predetermined set of best practices. Ie. Loosely coupled GUI, encapsulated data access, specific design patterns, etc.
   1. If these don’t exist (ie written down), then it is only personal coding style against personal coding style since to get here I passed 1,2 and 3.  If you have an architected system, then you can do code reviews against the overall architecture and everybody is on the same page. 
   2. A code review here would be extremely beneficial and can be proven as such since it ensures that everybody is coding according to standards. 

If you are giving anyone reasons 1,2 or 3 for code reviews, you are not going to convince anyone a code review will be worthwhile.

If you can’t give reason 4, then me (as the stake holder),  will say:  “We don’t have the time”, “Nobody is prepared to do the job”, or my personal favorite “Its unnecessary”, because it is.
 
I had a shot at answering these points so I thought I'd share them with you:

1. Do your coders produce code that does not build?
   1. Why waste time after the fact and wait for a nightly build to break and then have to “admonish” someone.  Its far simpler to spend 10-15 minutes with someone twice a day and make sure everything is great before. 
   2. You must have heard that the longer and issue lives the more expensive it is to deal with. 
   3. Reprimanding people after the fact is like smacking a dog the next day, doesn’t work, never has.  Much better to do a peer review and give praise for good work and help where its needed.
2. Do your coders produce code that does not work in the first place?
   1. Some do and the only way to help them is to give them peer support/reviews on a regular basis like every check in.
   2. You hired them.
3. Did your team build something in manner inconsistent with our systems.  Did they introduce dependencies that we had not planned on.  Did they use libraries, third party tools, etc not compatible with our environment?
   1. Code reviews don’t necessarily happen after the fact, they should also happen before any code is written so the lead developer/s can make sure the team starts off in the right direction.
4. Did they code not following a predetermined set of best practices. Ie. Loosely coupled GUI, encapsulated data access, specific design patterns, etc.
   1. Yes they did but without a code review how would you know?  You’ll only find out when its too late they have left and no one ever reviewed their code.
   2. Much better off having regular code reviews to keep morale up in the team.
   3. It also allows the leads to know who needs help, needs more guidance than others, and allows them to congratulate people when they deserve it.
   4. With regular reviews by the whole team there is a much greater understanding of the overall system by the team and thus a better result.

Overall this will save time and money and is therefore necessary.