Code Reviews

[Mac Sackett]

Hello,

I've been assigned the task of organizing code review sessions in my group at work. I would like to see some info on how best to approach code reviews.

Thanks
Mac

[Clearcode]

I have written an article on good programming practice at:
http://www.themestream.com/articles/309847.html

But I suspect you are more interested in the people side (how do I get the developers to go through this without bruising egos etc.)?

My only real comment on this is that it is a lot easier to get people to follow standards if they helped shape them - therefore getting your developers to brainstorm a coding standards guideline document for your organisation would be a good start.

HTH,
Duncan


-------------------------------------------------
Got "BigMetalHead"? You will soon
http://www.merrioncomputing.com

[thew]

The most important factor in a successful code reviews are

1) a good moderater..
he should be an advocate and keep the meeting on track. He should also be the chief defender of the code being reviews so the programmer doesn't have to be!

2) Who you invite to the meeting, and what their roles are...
No use in even having a code review if the people doing the review are junior to the person who coded the algorithm. Senior people should review junior coders.. Junior people can not contribute to a code review on the review side.. All they can do is mess things up.. If junior people are invited they should be there to learn and not interject themselves into the dicussion.. If they are there to participate in the review, you're lost before you begin..

3) Any time anybody says in a code review "this is more portable" they should have to stop talking.. SCOPE!! fine if your talking about code which needs to be portable.. But code which is never intended or designed to be portable shouldn't be nit picked with this barb which isn't in the design anyway!!

4) Any time anybody talks about optimization.. It should be expressly mentioned exactly what the optimization is saving.. All well and good if your optimizing harddisck hits which will improve the code performance by an order of magnitude. But saving of clock cycles or instructions of on a 2-5 mip machine should be excluded from discussion..

5) Optimization of bytes of memory.. ( ie... that's a 20 char field and you only need 15 bytes ) should also be excluded from discussion.. Worthless.. We've long since passed the point when such memory optimization had value or purpose. Even when it did have value and purpose way back in the early 640k days it was arguable self defeating..

6) overall reason for having code reviews is to spread knowledge and improve codeing practices. keep your eye on the ball and don't digress.. Moderator should rule with an iron fist... Exacto people should be controled and forced to sacrifice their speaking time to the task at hand..

[crupp]

Yesterday i had the first code review of my life :) It was not uninteresting, but i'm (student of computer-sciences) working at a hardware-company and so they have some different habits...

However, about your post:
I agree about everything but 3). I think portability is something which forces you to write your program on a higher level of abstraction. Therefore it gets a cleaner structure and you get reusable code. And reusability means cost-reduction, therefore i disagree with your point of view.

Just my 2 cents :)
Chris


----
http://www.crupp.de

[thew]

>> I think portability is something which forces
>> you to write your program on a higher level of
>> abstraction.

In theory you are correct. In practice I've found that someone who writes "portable" code has to end up recompileing that code on each and every platform which he desires it to be portable too. This is just beyond the scope of most projects. To truely make code portable is a significant and costly undertaking. What I object too is spouting off about porability of perticular function calls when the entire module was never and will never run on any other platform, Win32 or TCP/IP modules for example. It is just out of scope of the task which any proper code review should be focused on which is to obtain tighter code on the targeted platform.

Not that GUI or TCP code can't be platform independent, using #defines and given enough time you can make anything platform independent... mostly be rewriting it, but then your left with code which runs on two platforms but is harder to understand on either platform. Once again out of scope.