Proper Code Review

[Fuzemmo]

This is not a question of a specific code sample to be reviewed, but rather a question of theory and/or best demonstrated practices when it comes to performing a code review.

Currently, these reviews are completed in often less than two hours. What we are finding, is that this is simply not suffecient time to perform anything other than a superficial review. I'm looking to improve upon this process. I'm open to suggestions or redirection to some information that will help formalize this process a bit and allow it to add some real value to a project.

TIA,
Jim

 

[JBaileys]

Jim,

Are you looking for code review for SQL Server objects? Or also programming in VB and other related languages?

Here is a resource that we used on a previous project. It was nice that the software made performance recommendations. - And no I do not work for the company !?!

http://www.compuware.com/products/devpartner/1592_eng_html.htm

 

[Fuzemmo]

Most specifically, I'm looking at ways to improve the process of reviewing SQL objects. The code of procedures/triggers/views etc, the creation of tables and relationships, DTS.....

The same issues that I face in the SQL world also exist with the front-end developers. From top to bottom, I feel that our entire approach could use a revamp.

To some extent, I'm almost looking for a "definition" and/or set of "guidelines" as to what code review should be.

Jim

 

[TJRTech]

Review the section in Steven McConnell's book "Code Complete" that deals with code reviews. It should give you some good ideas.

Other things that you might find helpful:

1. Only invite those that need to be at the code review to the code review. It should NOT be a time spent educating managers and tech writers on languages, etc.

2. Consider pair-wise development (Extreme Programming coined the concept, but it has existed for years). Having two or more people that know, designed and can maintain a single piece of code (a module, library, app, etc) is a very good thing. Usually the code developed is better, less bug free, etc. When you have pair-wise, the code is being reviewe ALL THE TIME.

3. Consider tasking someone with the job of "code librarian" or "source control expert". This person could run the code reviews as a chair of sorts. They meet with the developer beforehand, get a feel for the project, code, etc. This removes the developer from the presentation of the code at the code review and is often a good thing for certain orgs were egos get bruised easily.

OR instead of 3....

4) Have the developers present the code at the review, but instead of a LINE BY LINE walk-through, have them review a prioritized list of functions, modules, sps, etc. based on several different categories, including: Most Complex; Most Troubling/Likely To Break; Most Tested; Least Tested (or Testable)

5) Make sure that unit test plans (test harnasses, test programs, etc) are part of the review.

Hope some of this helps.

Regards,
TR

 

[SQLSister]

TR's suggested book is a good one. Suggest you check it out if possible.

 

[Fuzemmo]

Thanks for the advice. The online reviews of "Code Complete" are consistent with what's being said here. I've just placed an order for the book and as well, have started a list of some things that I want to discuss with the group that I work with. For anyone who is in the same boat as myself, I thought I'd share that initial list, in no particular order...

* Set up CRs as structured, formal, recurring meeting (weekly?)
* Perform a CR by looking at printed code. Do not complete on-line.
* Code to be reviewed should be provided, along with Business Requirements and/or Technical Design considerations a minimum of two days prior to a review.
* Considerations for the review will include (A)adherance to adopted coding standards/conventions, (B)the code accomplishes the requested task, (C)the code is efficient, (D)the code is easy to trouble-shoot/interpret/maintain/enhance and (E)the code is well documented.
* A minimum of 2 reviewers will be required for code that is complex/difficult to test/critical functionality
* The CR session is not to be used as a formal education session to management, etc...
* The CR process should "blur" the concept of code ownership.
* The CR process should be an exercise in humility - no egos involved
* "N" coders/reviewers will have "N" ways of doing something. Recognize that a proposed method may not be your way of doing it, but if it meets all of the criteria above, than it should be deemed acceptable.

Thanks again to those who provided input.
Jim

 

[donutman]

A tweak in

"* Considerations for the review will include (A)adherance to adopted coding standards/conventions, (B)the code accomplishes the requested task, (C)the code is efficient, (D)the code is easy to trouble-shoot/interpret/maintain/enhance and (E)the code is well documented."

might be to assign one person to each of (A) thru (E) or at least a mutually exclusive list. Have them rotate their area(s) of concern.
-Karl