Code Review: a “pretty good practice”

What with recent events and all, it doesn’t look like Nick Hodges is going to get around to finishing up his Pretty Good Practices series anytime soon.  So I hope he doesn’t mind if I post one of my own.

Code Review is a very useful principle for keeping bugs out of the codebase.  At work we’ve got a rule which is (usually) followed pretty strictly: Any non-trivial change that might affect functionality needs to be reviewed by another developer before checkin.  Usually this is just a “rubber stamp” sort of thing.  You pull up a diff view against the source control version in Beyond Compare and show the other guy what you changed, he says “OK, looks good, go ahead and check it in,” and you go ahead and check it in.  But every once in a while they’ll catch something.

It can be just about anything, from debug code (commented out or left active) still in the code you wrote, to formatting problems that make it harder to read, to “I really think you could do this better if you were to change X and Y”, or even “that looks like it won’t work if the user does X.”  Occasionally the reviewer will even catch a syntax error that would have broken the build if you’d checked it in, either because you didn’t test it by compiling first or because you forgot to save your latest changes in the IDE.

It happens to everyone.  I’ve found all of these issues in my coworkers’ code, and had them find all of them in mine, at one point or another.  And when these sorts of things do get caught before going into the official codebase, it improves the quality of your program.  This is basically a small-scale version of Linus’s Law.

But a formal code review is not the only way to apply the principle.  You can catch a lot just by reviewing your own code, especially in a diff viewer like Beyond Compare.  Seeing what’s changed highlighted in red gives you a slightly different perspective than just looking at the ordinary code view, and sometimes it brings a mistake to your attention.  I usually check the diffs myself at work before asking a coworker for a review, and I always check diffs before checking any code in to the TURBU project repository.

Independent review can also be useful in actually finding the problem you’re trying to solve.  Maybe once or twice a month, I get a “hey Mason, can you come look at this code with me?  I need another pair of eyeballs” request from someone at work.  Sometimes I can help track it down, sometimes I can’t, and sometimes I just provide a useful suggestion.

I remember one time someone asked me to help track down a data corruption issue.  When I started looking at the code, it was a really ugly implementation of an array-based stack that someone else had written long ago, using pointers and GetMem/ReallocMem calls all over the place.  Looked like something straight out of my Data Structures class in college.  We’ve got a joke at work that a bad programmer can write C in any language, and that’s exactly what this looked like: someone writing C in Delphi.

At least there was a well-defined interface encapsulating this whole mess, so I said “I don’t think I can trace this very well, but it’s probably an out-of-bounds issue.  Any good reason why you couldn’t rewrite this with Delphi’s dynamic arrays and let Bounds Checking find the error for you?”  He did, and sure enough the problem became obvious real quickly.  It was trying to do a Pop on an empty stack at one point, and the debugger pointed out exactly where once the compiler was able to apply error checking to the problem.

Reviewing your code, and having someone else review it if possible, is a good way to catch bugs that might end up slipping into the codebase unnoticed otherwise, and it can also be helpful a lot of times to track down tricky errors if someone else is watching the debugger with you.  If you don’t do it in your development work, try it and see if it makes a difference.  It’s sorta like a seatbelt.  Most of the time it won’t do much for you, but when it does it’s very helpful.


  1. Dave says:

    Thanks for picking this series up, sort of. I liked PGP a lot.

  2. My best result with code review were when I had to explain to another developer the issue I was trying to correct, what was my approach to correct it and then go over the code explaining the implementation of the correction. Sometimes I would catch mistakes on my correction just by saying out load. And sometimes I even devised better ways to implement the fix. The other developer was there as a sounding board.
    The worse ones where when looking at diff between two versions with lots of minimal changes where the other developer did not care to elaborate on what his thinking path was to make code changes.

  3. Mason Wheeler says:

    Dave: I’m not really “picking up the series,” I just thought I’d throw this one in. Maybe other Delphi bloggers can add some Pretty Good Practices of their own?

    Alan: Good point about bad reviews. When it’s not clear, I like to ask, “what am I looking at?” If you ask questions, you’ll usually get an explanation.

  4. Jolyon Smith says:

    @Alan: Every developer needs an acolyte to explain themselves too for precisely this reason. Often it’s not another pair of eyes that helps, but a pair of ears. 🙂

    I myself have a “familiar” – a “Toni da Pony” Ferrari pony mascot keyring by my monitor. Talking to him out loud might get me put away, but as long as I keep my one-sided dialogues in my head nobody else can hear my crazy ideas. 🙂

  5. Michael Thuma says:

    With the help of LL(almost 1) languages as Delphi, Java and C# the situation has improved a lot and moved the “code review” more into the direction of two people thinking over – if the design and the implementation are still adequate and match the underlying problem to solve. This is why it is wise to express algorithms in an readable fashion.

    My point on Delphi is and this was for a certain period one of the few alternatives from the whole concept helps little more with the bottom up aspect of development. The whole IDE behavoir and the little more strictness concerning the lanuage force correctness in the detail at an earlier point in time. As long as people do not recompile every line typed …

    C/C++ non Algol based languages typically LR(n) are short to read and even if the symbols are defined readable and consistent the long compilation cycles fore writing a lot in a bunch and test in a bunch as well as VB or PL/SQL. Same is with classical web … is effective if it works in the first shot.

    We have good experience outside the code review with pair programming. We pair people up to learn from each other, review the code and implement requirements finished in one shot.

    The idea is nice – I am not sure if one needs the full power of the Code Collaborator for this. Agree here … discussing about the difference and explain what happens is apporpriate – we did it these das via Winmerge. A merge process is forces rethinking too.