Code quality notes

During the last term, I’ve been coding quite a bit again. Read on for some stuff to keep in mind when writing code, and for some things you better avoid.

War story one: I was debugging some math class which was using – somewhere deep within – Matrix::trace, which computes the trace of a (quadratic) matrix. Well, all good, some unit tests showed that the class seemed to work but in one special case, the result was wrong. I immediately thought that I’ve hit some corner case of that particular algorithm and I’ll have to add loads of unit test to get close to it, but after half an hour of debugging in turned out that:

• The class was totally broken actually, and the reason that it worked was …
• that the trace function was computing the trace and instead of returning it, it had a return true in it – which returned 1.

By chance, the matrices I had used for the first unit tests had indeed a trace of one. Be aware of seemingly trivial functions, as errors in those functions are extremely hard to spot. They are easy to test, so just do it!

Copy & paste

Just as in this case: While refactoring some code at work, I came to a function which was processing a vector. The vector was implemented using four named floats, so you had to write vec.x to access the first element. The code in question was just copying from one data set to this vector. The source data was a float array though, and you can probably guess what happened. I wrote something like:

vec.x = source [0];
vec.y = source [0];
vec.z = source [0];

Well, just a simple copy & paste error, but still, this could be easily avoided by providing:

• Index-based access, in that case, a loop would have been the right thing to do here.
• An assign function which takes a float array.

Copy & paste errors can be easily avoided by the interface design, as they are usually a sign of a missing helper function. For the record, I did a similar mistake just a few lines below, so this is really something that you should guard against ;)

Header mayhem

Headers are dangerous, especially, if they come from 3^rd party libraries. I can only recommend to wrap each such header in a custom header, so you can easily

• turn on/off warnings for this header. Some headers are pretty badly written and tend to emit lots of warnings when compiling at high warning levels. This is no problem if you turn off the warnings before including the header and turning them on afterwars.
• #undef stuff. Library headers tend to #define many things. Get a chance to clean up directly afterwards!

When including, include the header for the class you are implementing first, then C/C++ library headers, then your external headers, and then your project headers. Using this order, you immediately spot problems caused by external headers, which will save you quite some time looking for that mysterious “it-breaks-when-windows-h-is-included-somewhere-first” problems where, for example, the token ERROR gets defined.

NVI, interface checks

Prefer to use non-virtual interface classes, that is, interfaces where the implementation is virtual and private, but the client functions are public and non-virtual. This allows you to add checks for every function into the interface, saving you from checking again in the implementation. Less duplicated code, and each derived class immediately benefits from the base class checks. For debug mode, enable extended checks that also catch invariant violations etc. Additionally, you can easily provide helper functions without touching the classes implementing your interface.

For free functions and other classes, provide runtime checks to validate the parameters, together with a #define to turn them off globally (you might need this switch for release builds). Basically, if a function takes a pointer, check if it is 0, if you call something, verify the result is not broken, etc. I tend to have two macros, CHECK, and ASSERT, with CHECK being also enabled for runtime builds. As soon as a profiler tells me (a profiler! Not that colleague that says “this is going to be slow, I can feel it”) that it causes a problem, I change it to ASSERT. This catches API abuse immediately, and by issuing a __debugbreak();, your Debugger will show you directly where it happened.

Test

The single most effective thing to get your code bug free is of course testing it. I cannot stress this enough, but by testing your code extensively, you get better API design, robust code and improved flexibility. If you have to change something later, and your code has good test coverage, you can be confident that you didn’t break things. Just remember to run both unit tests and larger ones (integration, functional, you name it).