Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I am in the process of creating a short check-list for peer code reviews. So far I have come up with the following...

  1. Is there a unit test?
  2. Is there a integration test?
  3. Do you understand the code you are reviewing?
  4. Is it easy to maintain (simple,robust)?
  5. Does the code not duplicatie existing functionality ? (Don't reinvent the wheel)
  6. Does the code follow (my company's) naming conventions? (variable names,test class names etc)
  7. Are all functions, methods and classes documented? (Summary,parameters,comments etc)

    What do you think of this list? Did I miss something? Is there any standard?

share|improve this question

migrated from stackoverflow.com May 16 '11 at 8:45

This question came from our site for professional and enthusiast programmers.

3 Answers 3

up vote 10 down vote accepted

We maintain a code review checklist which omits things such as styling and other stuff that can be automated. The focus is more on ensuring that the functionality is achieved and code is in a maintanable state. To this effect, we have all of the items that you have mentioned. What we have is a fluid list i.e the checklist is updated every once in a while when a behavior appears or disappears (what I mean by that is if all of the developers are running findbugs and Unit tests before committing we remove it off the checklist but if we observe that the methods are longer than let's say 25 lines in a number of reviews, we then add it to the list).

So, my recommendation would be to start off with a list based on what is observed in your dev environment and tweak it accordingly when there is a need. Here are some of the items we have in our list (all of these are based on our dev teams code reviews)

  • Are there magic numbers in the code? Need to be converted to constants
  • Did the method name convey it’s intent Or variable name convey it's intenet
  • If there are queries, are they using parameter binding (use of prepared statement)
  • Are the variables declared at the narrowest scope possible?
  • Have all resources that are used returned back correctly – db connections closed, files closed etc
  • Does the code cover the story/task in question completely
  • Are the number of parameters in the method less than <4>
share|improve this answer
    
+1 for automating the dull parts of the code review process –  Gary Rowe May 16 '11 at 9:07
    
Thanks for your comment. The 'Does the code cover the story/task in question completely' question we have in our functional review which is done by tester. –  Gluip May 16 '11 at 9:12

I think you have missed the most critical thing - does it do what the requirement asked for. It doesn't matter how well written the code is if it doesn't match the requirement. I always start a code review by opening the requirement to the pertinent section and making sure it does what was asked for.

share|improve this answer
1  
I agree with this. I like to conduct code reviews where stylistic and trivial comments are banned for the first, say, 30 minutes. It's always frustrating to see a code review devolve into a style-bash fest and completely miss the actual bugs. –  Kevin Hsu May 16 '11 at 19:00
    
You are right...it is part of the review...Also the code should not do more than asked for. –  Gluip May 20 '11 at 18:53
    
Well for me the biggest reason to do this it to check the devs undersatnding of the requirement. I'm amazed at how often what they think the requirement means and what it actually says are not the same thing. SOmetimes they are right and the requirement is not written correctly but often they are just worng and it needs to be fixed. They also often miss what seem like small unimportant exceptions but which are a big deal to the client. This is especially true when the dev is unfamilar with the business domain. –  HLGEM May 20 '11 at 18:59

This is an example of Symbian S60 code review checklist that was used as mandatory input into both peer review and formal inspection processes applied at Tieto, Telecom division in 2008

Outputs of the code reviews could influence the contents (and priorities of the points) on this checklist.

Beside this simple list of points we also had more detailed descriptions of what the points mean with examples, rationale etc. So if in doubt anyone could read what the simple bullet exactly contains. This was basically compiled from the thick books we had/should read about Symbian OS.

The goal of this checklist was that errors frequently made/found in the past will not occur again in the future.

An important output of the inspection process was a list of errors found, their severity and the number on the checklist.

This was used for measurement of the review process and of the code quality.

Increasing number of errors of certain number could be improved by using a better macro, more easy-to-use class or by arranging a training on the troublesome topic

Question "does the code what it was supposed to do? does it solve the customer's problem? does it have to be that complicated?" was of course also part of the universal language-independent check list. A good example of such universal checklist is available in Steven C. McConnell's book - Code Complete - online

1. General Issues

1.1 Conventions and Naming

1   The code follows agreed coding conventions.
1a  Indentation, four spaces used instead of tabs.
1b  Spacing, no space, single space, parentheses…
2   The code matches the information in the design document.
    • Implementation matches the class diagram.
    • Dependencies match the component environment described in design document.
    • Dynamic behaviour of the code matches the operations in design.
3   Correct file templates have been used.
4   Descriptive “natural” names have been used for classes, functions, variables etc.

1.2 Structure and Comments

5   Maximum source code file length is 2000 LOC. Deviations need to be explained.
6   Maximum method length is 100 LOC. Deviations need to be explained.
7   There are no similar functions that could be combined. The code is unique.
8   There are no unused methods or member variables and code commented out.
9   Library functions are used where feasible instead of re-implementing the methods in the code.
10  The code behaviour is commented in a level that the intended behaviour is clear to the reader. ToDo comments are allowed only when the code is not final at the inspection time.

1.3 Variables and Constants

11  All variables are properly initialized.
12  All variables are defined in the smallest possible scope.
13  All variables are of proper type. No implicit casting allowed.
14  There are no literals or variables that should be constants.
15  There are no magic numbers (i.e. numbers other than 0 or 1) in the code. Use constants with descriptive names.
16  There are no hard-coded drive letters (e.g. E: for memory card). Use system libraries for drive and path information.
17  If possible, use references instead of pointers. All non-owned objects should be accessed via references.

1.4 Functions

18  Every method in a public API checks parameter validity before usage.
19  Units of parameters and arguments match, e.g. millimeters vs. inches. Details are specified in header file.
20  A method has only one return point. Deviations need to be explained.
21  The completion status of a method call is always checked and proper handling applied in case of an error.

1.5 Classes

22  Each class has an appropriate constructor and destructor. Members are initialized in constructor list.
23  Access to class members is restricted enough. Public members should be avoided.
24  Derived classes have no common members that should be in the base class.
25  Multiple inheritance is not used, except in case of inheriting interface (M) classes.
26  The public API of a class is designed so, that it's impossible to break the class and very hard to use incorrectly.
27  The keyword const is used correctly.
    • Each getter is a const method.
    • Each setter uses a const parameter.

1.6 Computation, Comparison and Control Flow

28  Overflow and underflow are prevented. 
    • The validity of integer variables & parameters is always checked before usage.
    • The size of strings or buffers is always checked before adding or removing data.
29  Division by zero is prevented.
30  Checking of boundaries (e.g. “less than” vs “less than or equal”) and comparisons ( e.g. == vs = ) are correct.
31  Priority rules and brackets are used correctly for getting the desired output.
32  The temporary variables needed in a loop are initialized before the loop and the loop itself runs only as long as needed.
33  Each switch statement has a default branch with a sensible implementation. If no implementation is needed, add a comment.
34  Switch statements are used instead of nested if statements where feasible.

1.7 Input and Output

35  All files are properly opened before use
36  All files are properly closed after use, also in case of an error
37  Resources (e.g. network, files, servers, memory) are allocated as late as possible, only on demand, and they are released as soon as possible.

1.8 Debug Logging

38  Debug logging has been implemented in the code.
    • All components have either traces (Rdebug::Print) or use log file.
    • All components log information in the same format: time component class method msg.
    • All implemented methods have logging, at least in
39  Debug logging is implemented in a way it can be disabled (compile or run-time) in release builds.

2 Symbian C++ Specific Issues

2.1 General

40  Object ownership transfers are used only in performance-critical places.
41  All dynamic memory is allocated on on-demand basis. De-allocation is done as soon as possible.
42  Timers are used only when absolutely necessary.
43  Each C-class is derived from CBase (either directly or indirectly), and the C-class is mentioned before derived M-classes in class declaration.
    • Failure in this issue will lead to memory handling faults especially when pushing an instance of this class i

2.2 Descriptors, Buffers and Arrays

44  Strings are primarily transferred by using TDesC&; copies are taken only when absolutely necessary.
45  Descriptors supporting Unicode are used. 8-bit variants are only used when absolutely needed.
46  Correct descriptor type is used for each use case
    • E.g. big buffers must be allocated from heap
    • E.g. constant buffers are used when there’s no need to change the data
47  Correct array type is used for each use case
    • E.g. flat types are used for arrays that are not changing frequently
48  Unnecessary use of Des() -function when calling a function that takes const TDesC& as parameter is avoided:
    • If you have a variable HBufC* buf, and function Foo (const TDesC& aDes), it is not necessary to  write Foo( buf->Des() ), instead write Foo( *buf

2.3 File System

49  Files are kept open only when needed. Exception: In case of frequent file access, it's better to keep the file open.
50  Each process has only one File Server session open.
    • In case of applications, a reference should be asked from the control context. 
    • Servers and other processes can establish a new session, but they must pass it on to other users, .e.g. DLLs.
51  Whenever writing to a disk, the availability of disk space is verified.
52  Write operations are implemented as transactions: they succeed or fail completely.
53  If using the file system fails, the software recovers from the error with appropriate error handling, such as an error message.
54  The code using files is aware of and can handle un-mount events of removable drives.

2.4 Robustness

55  All non-member pointers are asserted for NULL
    • Use a custom panic function logging the panic code before calling User::Panic."
56  All non-default cases and branches that should never be executed are asserted
57  __ASSERT_DEBUG is used instead of __ASSERT_ALWAYS
58  A custom panic function is used that logs the panic code before calling User::Panic.

2.5 Exception handling

59  All leaving methods are marked with trailing L.
60  Code design must avoid excessive use of TRAP. 
    • Catch if the leave needs to be catched in order for the calling function not to leave
    • Catch if there is a recovery defined for the error, otherwise let the leave propagate to a higher level error handler
61  Exception error handling is primarily done by using the clean-up stack.
    • Use the different CleanupStack-variants, like CleanupClosePushL.
    • Use custom cleanup items to handle exceptions that require more handling than the default delete or close.
62  Member variables are not pushed into clean-up stack; they are always deleted by the class destructor.
63  Local variables are not pushed into clean-up stack if there's no leave; it's useless and makes the code harder to read.
64  When using TRAP_IGNORE, a reason why it is used is provided as a comment.
    • Explain why we CAN ignore the leave.
    • Explain why we CAN'T or SHOULDN'T handle the leave
65  Functions should not leave AND return an error value, if avoidable.

2.6 Asynchronous Programming

66  Asynchronous programming is used in all tasks involving waiting.
67  Each Active Object is added to the Active Scheduler.
68  Each Active Object is correctly cancelled regardless on the state of execution
69  Avoid using big functions from RunL. Incremental execution is used with long lasting tasks.
70  If a leave in RunL can be handled, RunError is implemented to handle it with a return value KerrNone.
71  Asynchronous API can be wrapped to be synchronous only when reasonably justified in comments and CActiveSchedulerWait is used.

2.7 Localization & Resource Files

72  All UI strings are placed into localization files and read from a resource file.
73  Resource file reading takes the language selection into account.
    • In case of applications, this is automatic.
    • In case of servers and other processes, BaflUtils::NearestLanguageFile or similar is used

3 Selected Issues When Not Using Code Analysis Tools

74  LC methods are trapped only in non-leaving functions.
75  The return values of Connect() and Open() methods are checked and error handling implemented.
76  Close is called in the desctructor for R member variables.
77  Member variables are assigned as NULL if delete is called outside the class desctructor.
78  Non-leaving methods are not trapped.
79  ReadResourceL is used for resource reading
80  No leaving methods are called before BaseConstructL.
81  No code is leaving with KErrNone.
82  CEikonEnv::Static is not used frequently as it is a heavy function.
83  User::After should not be used. Usage needs to be justified in the comments
84  _L literals are not used in effective code. They are accepted for debug logging or test code only.
85  UID usage is planned. The same UID can only be used several times when that is required by the use case.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.