Something which comes up far too regularly in programming is that
people cannot read one another's code. This is a terrible shame,
because when you get right down to it, there's nothing more important
in the elimination of bugs than being able to tell what a program is
supposed to do. It doesn't really matter if the program does the
right thing today if it's unreadable, because as soon as
someone tries to change it, they will have no clue how it works and
will introduce new bugs. Thus the thing to remember is to make code
as readable as possible. It is the #1 goal.
Code Review is something too few projects ever encourage, but which
is perhaps the single greatest way of noticing and killing bugs,
increasing overall understanding, fixing design problems, and
learning from one another. The process is really simple and you will
be doing a Good Thing if you do it, regardless of your
background or skill set. Just follow these simple steps:
Set aside enough time. Rushed code review is worthless code review.
If you're new to libUFO, to C++, etc. then take a couple hours aside.
Choose a file in CVS. Any file will do. If you want to do something simple,
check out a header file. If you want something more complex, check
out an implementation file. If you're feeling ambitious, take a few
implementations in a related category all at once.
Either open it in a buffer with a scratch buffer to make notes in, or print
it out and take it to a coffee shop with a red pen. Mark down anything
you're suspicious about. You are not attacking the person you're reviewing,
you're helping them to see where their code could be better. Any commentary
you can make is of assistance, so make lots.
Check over the basics. This much you can do with hardly any understanding of what
the file is or what it does:
- Is there a header guard? (
- Is there a copyright notice?
- Is there a comment giving a general overview of the file?
- Does the file include only those headers which are strictly necessary?
- Does the class declare a virtual destructor and a copy constructor? (it should)
- Are there non-inline methods defined in the header? (there shouldn't be)
- Are there annoying, abnormally commented-out blocks of code with no explanation given?
- Are there obsolete
//!!!FIXME!!! comments lying around?
Do the comments accurately explain what's going on or have they lagged behind the actual
work done modifying the code?
Check out style. Again, this is mostly "eyeball" work which comments on the precognitive
noise that comes along with each chunk of code you're trying to decipher.
- Does the code use namespaces? We'd like everything to be tucked away in namespaces eventually.
- Are the names descriptive?
Methods should be short, meaningful verb phrases, such as
cookEggs, as opposed to
timerCheckOpen. The previous two, you can tell what they do from the names.
The latter two, you are left guessing. Methods which return boolean values should be phrased
like questions, such as
Formal parameters should be obvious from their datatypes if possible,
so the datatypes should be descriptive.
np is not nearly as useful as
Node *, if you get the idea.
Object-local variables should be prefixed with "my_". It's a simple step to discriminate and
it much more readable than "__" on the front. Besides which, it encourages you to write the
variable names as noun phrases, which they should be anyway, like
Formal parameter names can be short (like "
n") so long as the method is
(a) short enough that you can see the whole thing on 1 page, (b) the meaning of the
parameter is obvious from the method name and the parameter type. A method called
int activate(int p,int q) doesn't tell you diddly about whether the body
of the function is really doing what it's supposed to. A method called
void insert_element(void *o, container *c) is acceptable since no
misinterpretation is possible. If the method is longer than a page or so, a meaningful
parameter name becomes a necessity, as anyone reading it will have to thrash up and down
to remember what
vp refered to.
- Do the lines all fit on a reasonably sized page?
- Is there enough spacing to make it reasonably legible?
- Are the methods reasonably short (say, under 150 lines)?
Does the author overuse "clever" multi-way boolean expressions,
ternary operators, autoincrement and dereference operators, etc.?
Check out basics of logical comprehensibility and correctness (this requires thinking,
though not necessarily deep understanding):
Does the method make it clear from the onset what it's trying to accomplish,
either through comments or (preferably) direct, simple, well-named statements?
Is it clear how multiple boolean conditions interact? I.e., is it really easy
to tell, given an arbitrary combination of true and false answers to the various
tests and conditionals, which course of action the method will take? Are all cases
even accounted for? Are cases clearly separated from one another or are they all
tangled up? Are there too many levels of nested conditionals to make heads or
tails of it?
Does the method do one single task as described in its name, or does it have some
hidden side effects such as setting flags, creating/deleting objects, sorting or
rearranging objects, caching or redirecting objects, etc?
Does it try/throw/catch meaningful exceptions, and is it obvious which exceptions
will cause which course of action? can you see an easier way to structure the
Are there suitable checks against NULL values or thrown exceptions where it seems
possible that they might occur? Note: any pointer dereference is a potential
If the method touches (including reading) any internal state of an object,
is there a thread guard in place to prevent concurrent access?
Does the method cast down an inheritance hierarchy using
Does the method alter its parameters? If so, does it need to? If not, can the
parameters be made const?
Does the method alter the state of the object? if so, does it need to? If not,
can it be made into a const method?
Can you foresee a case in which a programmer would want to extend or override
the method? If so, make the method virtual.
Does the method need to exist at all? If so, does it need to be expressed through CORBA?
If not, does it even need to be public? If not, can it even be made private? If it is a
utility method, can it be made static?
Does the method fail gracefully when there is no more memory, when a pointer is null, etc.?
Or does it just leave the door open to disaster?
Does the constructor use assignment when it could get away with using initialisation?
(initialisation is better)
- Does the method return object-internal data by reference? (this is bad)
- Does the code reveal a public data member of an object? (this is bad)
- Does the code use
#define where it should be using
- Work your way through what it's actually doing and see if you can find major logical flaws:
- Are there resource leaks you can spot? Open files, memory leaks, etc?
Are there possible thread deadlocks? A deadlock happens when
two threads are blocked waiting to acquire locks one another
- Is it possible to produce invalid iterators or cursors?
Can the code be made less dependent on the contents of other
files, either by using implementation superclasses, or CORBA
Can you think of any general way of making the algorithms more
readable or simpler?
Can you think of a case where you see two things doing almost
identical activities, which could be factored into a common case
Once you have a bloody, war-torn page marked up with the above commentary,
you have 2 options:
The latter is much more likely to actually work. If you get no reply at all
from the original author, it's possible they're dead, and you might need to
ask someone else with CVS write permission to commit it for you. If you're
going to be doing a lot of review, you should probably get in touch
and we can set you up with permanent write access.
Mail your commentary back to the author and wait for them to
get around to fixing things.
Try and correct the problems, even just a few of them, and mail the
author a patch as well as some commentary on what you did and why
you did it.
This description was shamelessly stolen from the project.