The topic of this post sorts reminds me of the whole DbC mess that paralyzed our group for years. Wow, the fallout from some key bad decsions! Well, this example is much more harmless, but I thought it was interesting.
A tester discovered that the SQM code wasn’t achieving the correct code coverage numbers. The target is normally 85% and the SQM code was hovering somewhere around 60%. Why was this?
Because there was a lot of code that looked like this:
if( FAILED( sqmInitStatus_ ) )
{
HWSW::Trace( L"SQMSession::Initialize:: SQM temporary file path error: %s", pSqmFileLocation );
return sqmInitStatus_;
}
Normally, getting inside an “if” statement like this is quite easy from a unit test. You simply have to write a test that forces that error condition. Then, testing your error handler is a simple matter. The only problem was that the author used a traditional method for validating string arguments at the beginning of functions. These traditional validation methods are fine for string validation when you’re using functions like strcat and strcpy. But, if you’re using the safe string functions (StringCch*), then is it overkill?
if ( pAppVersion == NULL || ( ::wcslen( pAppVersion ) == 0 ) || ( ::wcslen( pAppVersion ) > MAX_VERSION ) ||
…
If these any of these conditions fail, the function will return an INVALID_PARAMETER return value. It’s therefore basically impossible for the StringCch* functions to fail, which makes getting inside those if statements impossible.
So, what should be done?
My opinion is to modernize this sort of code. The code does not need so much argument validation at the beginning because the StringCch* functions manage a lot of this. Also, writing unit tests early could have helped catch this sort of problem.
