The Joel on Software Discussion Group (CLOSED)

A place to discuss Joel on Software. Now closed.

This community works best when people use their real names. Please register for a free account.

Other Groups:
Joel on Software
Business of Software
Design of Software (CLOSED)
.NET Questions (CLOSED)
TechInterview.org
CityDesk
FogBugz
Fog Creek Copilot


The Old Forum


Your hosts:
Albert D. Kallal
Li-Fan Chen
Stephen Jones

Code Review

hello everybody,

I would like to know from experts here about how to do a good code review. I am suppose to do code review of a big .net project and I want your ideas on it.

thanks!!
hitchhicker
Friday, February 24, 2006
 
 
Have you considered FXCop? http://www.gotdotnet.com/team/fxcop/

Also check out this article:
 http://www.codeproject.com/dotnet/Cyclomatic_Complexity.asp

You can quickly identify which methods are the most complex, and work from there.
UK Techie Guy Send private email
Friday, February 24, 2006
 
 
How not to do it...

http://meet-joe-bloggs.blogspot.com/2006/02/episode-6-code-review.html

or even

http://www.techbookreport.com/jb/joebloggs24.html

There's even a Stob piece on it somewhere but I can't find it...
jbfan
Friday, February 24, 2006
 
 
semigeek
Friday, February 24, 2006
 
 
I'm assuming your organization has published coding standards...

You need to have the following roles for a review: Author, Reviewers, Moderator and Scribe. The Moderator doesn't have to be a technically-savy individual, or know the code. Their job is to keep the meeting on track and keep everyone focused on the task at hand. The Scribe is responsible for keeping a log of issues, action items that must be taken, and for distributing the minutes with the above. The Moderator is then responsible for making sure the action items are completed on schedule. Most organizations I've been in rotate the Moderator and Scribe responsibilities; usually these are people in QA or Requirements Management positions. You don't want the Author or a Reviewer playing one of these roles.

Automate as much of the review as possible. There are plenty of free and low-cost tools that can do style checks (for naming conventions, formatting, comments, etc) and lint-type checkers that look for common defects. Make it the responsibility of the Author to run their code through these tools AND resolve any issues before publishing for review.

Keep the reviews short and sweet. Make it a goal that the artifacts under review can be processed in under an hour. Make it the responsibility of the Reviewers to review the artifacts prior to meeting. The review meeting itself should take an hour or less, and should focus on issues the reviewers found, not parsing every line of code.

Lastly, maximize your review time by reviewing only the most critical code. If you can pull history reports from your version control system, look for files that have had a lot of checkouts/checkins in a short period of time; chances are the coder(s) may have had problems with the implementation, or the design is bad, or the requirements unclear. You should also have a feel for the most critical sections of your system; review any changes to code in these areas, or changes that impact external interfaces to other areas.
Former COBOL Programmer Send private email
Friday, February 24, 2006
 
 
hitchhicker

You need to find out, or decide what you are reviewing the code for:

All functions are commented correctly?
All third party library calls are protected with exception handlers?
Variable names follow the company convention?

Once you've done this you'll then be asked to write your company coding standard so no one else will ever need to ask this question again.
IanH. Send private email
Friday, February 24, 2006
 
 
hitchhiker,

I would recommend chapter 21.3, "Formal Inspections", of Steve McConnell's _Code Complete_, 2nd edition.

http://www.amazon.com/exec/obidos/ASIN/0735619670
PWills Send private email
Friday, February 24, 2006
 
 
Yes, go get McConnel's Code Complete 2 and base your standards on that.  While it probably won't be complete for your organization, it is a reasonable starting point.

Then pick up a book on common security exploits.  I know you're doing .Net, but Chris Shiflett's Essential PHP Security covers the basics and principles.  My Review:  http://blogs.caseysoftware.com/?q=node/187&title=Book-Review:-Essential-PHP-Security
KC Send private email
Friday, February 24, 2006
 
 
Good point on specifically reviewing security.

My pick would be anything by Gary McGraw. He writes for Java but, like Casey said, the concepts are the same in any platform.

_Exploiting Software_ was published *last week* and is therefore very up to date. (I'm not that fast a reader; I happened to have seen a pre-publication manuscript!)

http://www.amazon.com/exec/obidos/ASIN/0201786958
PWills Send private email
Friday, February 24, 2006
 
 
The excellent Applied Software Project Management has useful things to say about code reviews. The authors referenced some of them recently in their blog:

http://www.stellman-greene.com/aspm/content/view/56/43/
http://www.stellman-greene.com/aspm/content/view/55/43/

Along with a checklist from the book:

http://www.stellman-greene.com/code_reviews
Chris Winters Send private email
Friday, February 24, 2006
 
 
>> ... like Casey said ...

I meant to write "Mr. Casey". Didn't want to come across sounding like I was on a high-school sports team. ("Yo Casey I'm open!")
PWills Send private email
Friday, February 24, 2006
 
 
Here are some code review references:

http://www.possibility.com/epowiki/Wiki.jsp?page=CodeReviews

You don't need to do the heavyweight code review process. Use an asyncronous code review process to review every line of code before it is checked-in. It doesn't take much time and quality really improves.
t
Friday, February 24, 2006
 
 
The other thing is to watch your psychology and body language during the review.  Remember - any defects found are the codes fault, not the coder's.
example Send private email
Friday, February 24, 2006
 
 
I will never forgive you.


Seriously though, McConnell's Code Complete and Security review and you'll have a good starting point.  You'll also be about 100 miles past everyone else.
KC Send private email
Friday, February 24, 2006
 
 
"I'm assuming your organization has published coding standards...

You need to have the following roles for a review: Author, Reviewers, Moderator and Scribe."

and Grumpy, Dopey, Sneezy, and Doc.

Did you guys have little plasticized cards with the procedure printed on them?
DoneThereBeenThat Send private email
Friday, February 24, 2006
 
 
No, they were tatooed on the back of our necks.
Former COBOL Programmer Send private email
Friday, February 24, 2006
 
 
Thank you very much everybody. That was a great help!!!. I have enough details now to start.  I have also bought McConnell's Code Complete yesterday. :)

-hitchhicker
hitchhicker
Saturday, February 25, 2006
 
 
This was just posted to Hacknot:

In Praise of Code Reviews
http://www.hacknot.info/hacknot/action/showEntry?eid=83
Chris Winters Send private email
Monday, February 27, 2006
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz