C++ Code Review CheckList
Classes
1 Does the class have any virtual functions? If so, is the destructor non-virtual?
Classes having virtual functions should always have a virtual destructor. This is
necessary since it is likely that you will hold an object of a class with a pointer of a lessderived
type. Making the destructor virtual ensures that the right code will be run if you
delete the object via the pointer.
2 Does the class have any of the following:
Copy-constructor
Assignment operator
Destructor
If so, it generally will need all three. (Exceptions may occasionally be found for some
classes having a destructor with neither of the other two.)
Deallocating Data
1.Are arrays being deleted as if they were scalars?
delete myCharArray;
should be
delete [] myCharArray;
2. Does the deleted storage still have pointers to it?
It is recommended that pointers are set to NULL following deletion, or to another safe
value meaning "uninitialized." This is neither necessary nor recommended within
destructors, since the pointer variable itself will cease to exist upon exiting.
3. Are you deleting already-deleted storage?
This is not possible if the code conforms to 6.2.2. The draft C++ standard specifies that
it is always safe to delete a NULL pointer, so it is not necessary to check for that value.
If C standard library allocators are used in a C++ program (not recommended):
4.Is delete invoked on a pointer obtained via malloc, calloc, or realloc?
5.Is free invoked on a pointer obtained via new?
Both of these practices are dangerous. Program behavior is undefined if you do them, and such
usage is specifically deprecated by the ANSI draft C++ standard.
Constants
1. Does the value of the variable never change?
int months_in_year = 12;
should be
const unsigned months_in_year = 12;
2. Are constants declared with the preprocessor #define mechanism?
#define MAX_FILES 20
should be
const unsigned MAX_FILES = 20;
3. Is the usage of the constant limited to only a few (or perhaps only one) class?
If so, is the constant global?
const unsigned MAX_FOOS = 1000;
const unsigned MAX_FOO_BUFFERS = 40;
should be
class foo {
public:
enum { MAX_INSTANCES = 1000; }
...
private:
enum { MAX_FOO_BUFFERS = 40; }
...
};
If the size of the constant exceeds int, another mechanism is available:
class bar {
public:
static const long MAX_INSTS;
...
};
const long bar::MAX_INSTS = 70000L;
The keyword static ensures there is only one instance of the variable for the entire
class. Static data items are not permitted to be initialized within the class declaration, so
the initialization line must be included in the implementation file for class bar.
Static constant members have one drawback: you cannot use them to declare member
data arrays of a certain size. This is because the value is not available to the compiler at
the point which the array is declared in the class.
Links
"Code Review Checklist" by Charles Vaz - http://charlesconradvaz.wordpress.com/2006/02/16/code-review-checklist-2/
Code Inspection Check List - http://www.chris-lott.org/resources/cstyle/Baldwin-inspect.pdf
C Code Reviw Guide - http://casper.ict.hen.nl/se/SEscripts/CodeReviewGuide.html
Best Practices: Code Reviews - http://msdn.microsoft.com/en-us/library/bb871031.aspx
