Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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];
share|improve this question
1  
better, avoid checks like that; if you're calling [music stopPlaying], it shouldn't matter if some button is visible (that's view stuff) and if the music isn't playing, it should just do nothing –  sparkleshy Dec 30 '12 at 21:18
    
@sparkleshy: there may be a good reason to have a tight relation between the UI and the music stopPlaying. The following code may be simply within a method which is executed on button click, so while music stopPlaying is business logic, the actual piece of code is not. –  MainMa Dec 30 '12 at 21:27
    
@MainMa: yes ... if the method is executed on button click, then the button is visible –  sparkleshy Dec 31 '12 at 0:55
1  
@sparkleshy it's possible to fake button clicks, especially on windows! –  James Dec 31 '12 at 4:02
1  
@James: it's possible to fake anything (including the button programmatically registering as visible). besides, who cares if it's fake? –  sparkleshy Dec 31 '12 at 15:37
show 3 more comments

3 Answers

up vote 12 down vote accepted

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 makes no sense:

    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
share|improve this answer
    
Thanks for the quick response. I'll have to start thinking more outside the box from now on I guess. –  cory ginsberg Dec 30 '12 at 21:16
    
+1: Very good points. Some checks are superfluous in the current version of a piece of software, but become relevant if another programmer changes the code (or if you change it yourself, months later) so that certain conditions are no longer guaranteed to be valid. The safest way to go is to never assume anything about the rest of the program. –  Giorgio Jan 1 '13 at 10:10
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
1  
+1 Nice analogy! But I disagree. By adding a conditional check, you might have just covered up a bug (or blind spot in the analogy's case). Keep it simple, if it's not needed then don't code it. Adding code for possible future cases or coders is a form of future proofing. By writing simple code, you are enabling future coders easily grok your code. –  Chuck Conway Dec 31 '12 at 21:48
    
@ChuckConway: Thanks, Chuck. Please see my edit. –  Jim G. Dec 31 '12 at 23:06
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.