Would it be better to have extra checks, or would it be a waste of time?

Question

In your opinion, do you think it is a waste of time to make checks that you know there is no possible way of it being there/not being there, or would you just put it there just in case there is a bug or something? For example, below checks to see if a button is visible, then music is playing, but there is no way for that button to be shown without the music playing:

if (buttonVisible)
    if ([music playing] == true) //is always true if the button is visible
        [music stopPlaying];

Answer 1

What if:

• Some other application manipulates the window and renders the button visible while the music is not playing?

• A thread stops the music and is about to hide the button when you check the visibility of a button?

• Some other developer modifies the code, without knowing the relationship between the visibility of the button and the music state?

• You modify the code since the requirements changed and instead of hiding the button, you were required to only disable it?

• You replace the button by some other control?

In other words:

1. You shouldn't check for a condition if “there is no possible way of it being true/false”. For example the if block in the following code is redundant and should be removed:

   assert(input > 0);
   var i = input * 2;
   if (i > 0)
   {
       // Do something useful.
   }
   

2. You should check for a condition in a case like the one you illustrated, since there is no strict and reliable relation between the visibility of a button and the state of the music.

---

Note that in your code, what you should omit is the first if. Since you simply want to stop the music (and for a reason, you can't just call music stopPlaying if the music is not playing), the only check you should do is if ([music playing]). That the button is visible or not doesn't matter. You don't care whether it is visible or not, enabled or not, or even if it exists.

For example something like:

if ([button visible])
    if ([music playing])
        [music stopPlaying]
        [button hide]

could be rewritten as:

if ([music playing])
    [music stopPlaying]
    [button hide]

if the visibility of the button relies on the state of the music. Data binding, if supported by your language/framework, is even better:

[button visibility=(bind:music state)]

// Later in code:

if ([music playing])
    [music stopPlaying] // The button is hidden automatically

Answer 2

I'd liken these seemingly superfluous checks to activating your directional when turning on a vacant street.

On that vacant street, you've done your due diligence, scanned your surroundings, and checked your rear view mirror. There's nobody around, so why should you activate your directional?

Here's why: Because next time you drive down a similarly vacant street, you might miss a car in your blind spot.

The act of activating your car's directional communicates intent. Also, it's just a good habit to get into.

Similarly, I recommend that you include such checks in your code wherever you think they are applicable. These checks will communicate intent to future programmers who will read your code. And in light of the fact that your code will be modified in the future, you can't predict the context of future method calls.

---

EDIT: Per @Chuck Conway's comment, it is probably best to convert most of these if checks to either assert statements or if (not 'X') then throw statements. An encapsulating if statement might cloak a very unexpected bug!

Also, I agree with his point about simplicity. There's a fine line between adding some additional checks and adding junk that will confuse future programmers. So be judicious.

Answer 3

In your opinion, do you think it is a waste of time to make checks that you know there is no possible way of it being there/not being there, or would you just put it there just in case there is a bug or something?

There are three possibilities, it's implossible

int i = 0;
if (i==1 && i==2) {
 // useless test
}

It's possible but requires a change in the code somewhere.

int i = 0;
if (i>0) {
// line above should prevent this from happening.
// extreme case, if this branch is executing someone did something wrong
}

And finally it could simply be an invalid state, something which you don't think can happen given good data.

The first case is a total waste of time, no change to the code or the user input can result in that test succeeding. The second case is MOSTLY useless, but a variation on it can be useful. Basically, the second case as presented requires a change in the code to for that branch to be take, but a more complex example could be useful in validating an assumption that is baked into the following algorithim, but is not inherently enforced.

For instance, if instead of an int, it was creating a uninitialzed User, then a check that the name was empty could theoretically be valid -- the User could be changed so that a default name is supplied, and if a write once property was changed so that the default name could be changed, it could mess things up.

But really, there are better ways of testing for such things (unit test for instance, although that's not really what they are for).

Finally, there's the last case -- and whether you test for that depends upon what you can do about it that is useful. And just how unlikely you think it is -- if it is realistically never going to happen, then testing for it is a waste of time. If on the other hand, it shouldn't happen but can, then by all means catch it if you can do something useful.