What points should a code review check list have?

Question

I am looking to prepare a code-review checklist which reviewers should be using in validating the code they are reviewing. I am mostly working in C/DSP/ARM assembly language code. Towards that I have thought of certain points which would go in that check list such as:

• No magic numbers.
• Pithy comments for every block of code which is related.
• Variable initialization before using, etc.

I am looking for more check points to be added here which would make the entire review process critical and serious, useful(catching bugs, if any, early). Can you add from your experience?

Answer 1

I've only been on one project which did some code reviews, so I'll give you my meager experiences.

What we mainly looked for were:

Proper method names (nothing too ambiguous).

Ensure that a method will do one thing. No methods that have 50 different return paths.

Good, clean comments which include brevity.

Answer 2

Watch for = instead of == inside of conditional statements.
Of course properly formatting/indenting the code.
Things left hard coded, like some configuration option.

My apologies, it wasn't specified that 'simple' things such as '=' were already coved in the original version of the question. However it did (and still does) say, "Common concepts across different programming languages are welcome!"

Answer 3

• Boundary Conditions (what is going into and out of a method is what you expect e.g. no nulls)
• Code style follows company standards
• Code is not duplicated or copied (this can be automated Google for Simian)
• Code complexity - Google cyclomatic complexity
• Will the code that has been written cause any knock ons in other parts of the system
• If class, method and variable names are kept meaningful a lot of comments should not be necessary
• Is exception handling in place and comply with company standard
• There are Unit Tests for the code that are meaningful and the tests are relevant

I think it is important to keep code reviews informal, they should be a chat between the reviewer and the person who wrote the code.
Never be negative about the persons code the code review is there to help them write better code not isolate them. Saying things like "that is crap" can be hurtful a bit of tact is required.

Answer 4

In general, I look for the following in a code review:

• Are all methods in a class as short as they could be
• Do they have descriptive names
• Are those methods doing only one thing (actually the thing that their name says it does)
• Did somebody try to justify crappy code with some nice comments
• How well does the code read in general

In addition you should look at things like

• coupling
• cohesion
• thread safety (if threads were used)

If you have a chance, get a copy of Bob Martins book "Clean Code" before doing the review.

Answer 5

Normally, you have already some guidelines about function naming, structuring, style, etc. established in-house. So the code review should make sure that anyone that has to work with it can

1- understand what the code does

2- assert that the code is doing it correctly

You will have a human reviewing this, so I feel style, efficiency, etc. should be left to tools and tests that can get you quite far. But code clarity is almost impossible to measure automatically, and is paramount for maintenance.

Answer 6

In the end it all boils down to smell.

Answer 7

• Variables declared with the narrowest scope possible (function/method level vs. global/class level, and block level vs. function/method level). This seems to be the #1 cause of bugs in the code I deal with - variables declared at an excessively wide scope and allowing old values to stick around between calls.
• Variables declared at first use, not all the top of the function.

Answer 8

Conformance to your in-house coding manual - which may of course include some of the points mentioned here by others.

Answer 9

There is no way you should be spending developer time on coding standard issues - these should be handled as much as possible with tools: uncrustify / PCLint.

I would recommend reviewing code for:

• Logic errors
• Conformance to the specification (you have one of those, right?)
• Robustness/defensive programming

Answer 10

http://charlesconradvaz.wordpress.com/2006/02/16/code-review-checklist-2/

Answer 11

1.proper import satements i'e be sepcific what to be import 2.Inilization of variables should be done and nullify of variables in finally block 3.Correctly close the result set and connections after use. 4.Proper comments should be there for the new changes 5.Remove the unwanted variable 6.Methods and variable name should follow the naming convection of the application 7.Revision no should be there

Aseem