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.
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.
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
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.
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.