Code review

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:

1. 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.
2. 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.
3. 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.
4. Check over the basics. This much you can do with hardly any understanding of what the file is or what it does:
   1. Is there a header guard? (#ifdef _foo_h)
   2. Is there a copyright notice?
   3. Is there a comment giving a general overview of the file?
   4. Does the file include only those headers which are strictly necessary?
   5. Does the class declare a virtual destructor and a copy constructor? (it should)
   6. Are there non-inline methods defined in the header? (there shouldn't be)
   7. Are there annoying, abnormally commented-out blocks of code with no explanation given?
   8. Are there obsolete //!!!FIXME!!! comments lying around?
   9. Do the comments accurately explain what's going on or have they lagged behind the actual work done modifying the code?
5. 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.
   1. Does the code use namespaces? We'd like everything to be tucked away in namespaces eventually.
   2. Are the names descriptive?
      1. Methods should be short, meaningful verb phrases, such as attackEnemy or cookEggs, as opposed to preFormatMigrate or 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 IsProcessFinished or HasUserBittenCarrot.
      2. 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.
      3. 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 my_chicken_turnip instead of chtnp.
      4. 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.
   3. Do the lines all fit on a reasonably sized page?
   4. Is there enough spacing to make it reasonably legible?
   5. Are the methods reasonably short (say, under 150 lines)?
   6. Does the author overuse "clever" multi-way boolean expressions, ternary operators, autoincrement and dereference operators, etc.?
6. Check out basics of logical comprehensibility and correctness (this requires thinking, though not necessarily deep understanding):
   1. 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?
   2. 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?
   3. 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?
   4. 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 exceptions?
   5. 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 segfault.
   6. If the method touches (including reading) any internal state of an object, is there a thread guard in place to prevent concurrent access?
   7. Does the method cast down an inheritance hierarchy using dynamic_cast or static_cast?
   8. Does the method alter its parameters? If so, does it need to? If not, can the parameters be made const?
   9. 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?
   10. Can you foresee a case in which a programmer would want to extend or override the method? If so, make the method virtual.
   11. 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?
   12. 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?
   13. Does the constructor use assignment when it could get away with using initialisation? (initialisation is better)
   14. Does the method return object-internal data by reference? (this is bad)
   15. Does the code reveal a public data member of an object? (this is bad)
   16. Does the code use #define where it should be using consts?
7. Work your way through what it's actually doing and see if you can find major logical flaws:
   1. Are there resource leaks you can spot? Open files, memory leaks, etc?
   2. Are there possible thread deadlocks? A deadlock happens when two threads are blocked waiting to acquire locks one another holds.
   3. Is it possible to produce invalid iterators or cursors?
   4. Can the code be made less dependent on the contents of other files, either by using implementation superclasses, or CORBA reference types?
   5. Can you think of any general way of making the algorithms more readable or simpler?
   6. Can you think of a case where you see two things doing almost identical activities, which could be factored into a common case with parameters?
8. Once you have a bloody, war-torn page marked up with the above commentary, you have 2 options:
   1. Mail your commentary back to the author and wait for them to get around to fixing things.
   2. 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.
   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.

This description was shamelessly stolen from the project.

• Coding style