Say No to Redundant Checks

I had a long argument with a friend about some check that I don’t do in diStorm. He only said that apparently I don’t do that check (if statement to test some value.) But if it was not important enough he wouldn’t have said that in the first place, right? :) And I told him that he is right – I don’t, because it is absolutely not necessary. Soon I will let you judge yourself. Eventually we got nothing with that argument, probably both sides are mad (at least I was pissed off) and the code left the same.

Now let me show you an example where I will ask you if you do an (extra?) test of the result or not. Since I’m a Win32 guy, here we go:

char buf[256];
int length = GetWindowText(HWND, buf, 256);
if (length == 0) return ; // No string for some reason, though I don’t care why.

memcpy(DST, buf, length+1); // Include null termination char.
Well, that’s a useless code, but still good enough for the sake of conversation. So I get the caption of some window and copy it into another destination buffer. Did you notice that I did not check the length before copying to DST?! (And yes, DST is the same size as buf). Oh no!! Am I doomed now? Of course not, it is guaranteed that the length will be less than 256 for sure. Because otherwise the API is broken. Now we can start another flaming about who’s fault it is, but spare me this, please. So if the API is broken, it will be probably a bug, and will break other applications which use it. But that’s not the point. Say the API is fine, why don’t I check the length before I memcpy? Am I doing anything wrong? I will say it shortly: NO. My code is just fine (as long as DST size is at least buf’s size, of course). So his accusations were that I should be a code-monkey-freak and consistent. And then he said something like “And what if in the future the API changes?”, Well, bammer, all other API’s users are now broken as well. If it was for that question, I would have to pad all my code with extra unnecessary IF’s. No thanks. Besides if I won’t trust my API’s what should I trust? They are the basis for every appliaction.

Now in diStorm the case is the almost the same, there is some internal function that you call with lots of parameters, one of them is an output param that receives the size which was written to some buffer you supply as well as a maximum size of that buffer. So I examine the return code and see whether I can use the data in the buffer or not, and then I know that the size of written entries in my buffer cannot exceed the maximum size I supplied. So why the heck should I check that writtenEntriesCount < MaximumEntries? Tell me, please!

The only rational I can see is to make an assertion (and only in debug mode) that everything is correct. But that’s not a must, and not doing that won’t even let you the possibility to say that I am a bad coder. The thing is, that both the internal function and its caller were written by me, and both are internal only (not exported in any way) so as long as one of them (probably the called) guarantees that the written entries don’t exceed the maximum size. Everyone are is happy. If it wasn’t the case, I should have had a crash (buffer overflow, access violation), just something nasty, that will hint of the buggy function. Now we can get into the philosophical issue of whether the callee or caller should make that test, or both. But for me, as I see it, it’s better be put inside the called function since it must work well for the interface it supplies. And I always think ahead and ask myself “…and what if someone will call that function and forgets(/doesn’t know -) to add the extra check?”

Don’t get me wrong assertions are really useful devices, I really suggest you use them. It just happened that diStorm doesn’t have them. Maybe I should add them. And yet, it’s not a must.

Leave a Reply