Should you always write code for else cases that "can never happen"?

Question

Take some code like

if (person.IsMale()) {
    doGuyStuff();
} else {
    doGirlStuff();
}

(Yes, I realize this is bad OO code, it's an example)

Should this be written to explicitly check if person.isFemale(), and then add a new else that throws an exception? Maybe you're checking values in an enum, or something like that. You think that no one will add new elements to the enum, but who knows? "Can never happen" sounds like famous last words.

Answer 1

I think you've answered your own question. If you know you're never going to see additional values:

bool isSet = ...
if (isSet)
{
    return foo;
}
return bar;

...then don't bother. But if there's a chance there could be more than two possible values, cover yourself. You (or the maintenance programmer two years down the road) will be grateful for it.

Answer 2

I find 'can never happen' sounds good until it bites you in the ass months later, after a colleague has added code, breaking your original code. So, for myself, I would make sure my if 's are solid even if it seems impossible.

Answer 3

I guess it depends on your object type. If it is strictly boolean (or binary in general), then, as you guessed already, the second test is redundant. Any other case, the second test will be in place.

However, even under this condition, you can have a "mental shortcut" in protecting from future expansion of the object domain - just assume that only one value is the "true" value, and all the other values are defaulted to the "false" value.

Answer 4

If you read some of the formal methods books carefully, they suggest that you do this kind of thing.

1. Define the post-condition

   isMale && doGuyStuff || isFemale && doGirlStuff.
   

2. Derive some candidate statements that will lead to this post condition

   if isMale: doGuyStuff
   

   That provably leads to some of the post-condition

   if isFemale: doGirlStuff
   

   That's provably leads to some of the post-condition

   Note that order does not matter. Indeed, it's simpler if you remove any ordering assumptions.

3. You wind up with the following:

   if isMale: doGuyStuff
   elif isFemale: doGirlStuff
   

   Note that there's no proper use for an else clause. You'll never -- in a formal derivation -- derive an else clause. You'll always have conditions that are positive statements: a && b || c && d kinds of things. Rarely it will be a && b || !a && c, but even then, you usually wind up with the explicit !a condition.

Formally, the "impossible else" clause should be limited to doing something like the following.

    if isMale: doGuyStuff
    elif isFemale: doGirlStuff
    else:
        raise HorrifyingSituationError

Should you ever raise the HorrifyingSituationError, it means that you did the math wrong and improperly derived the statements from the conditions. Or you improperly defined the post-condition in the first place.

Either way, the program was designed wrong in a profound and absolute way. Generally, this is not a surprise. It usually fails spectacularly the first time you try to test it. Unless (as it often happens) you chose test data that reflects the errors in your original definition of the post-condition. Even then, once you encounter this exception, you can easily track it down and fix it permanently.

Answer 5

@Michael Petrotta - your code snippet is incorrect, as the DoY() action is taken disregarding the truth value of the condition.

(sorry, can't add comments yet...)

Answer 6

I don't get to do coding for my company but there have been many instances where our programmers have coded things for that case that will never happen and it has come in handy when trying to troubleshoot customer reported problems. It seems to happen every so often with code we get from our overseas projects.

When a customer calls and says "Hey I am getting a "type not allowed error" and it says "type duck not allowed" we have quickly found the cause of the problem and been able to resolve it.

Answer 7

If it can never happen then you wouldn't need to write code for it.

Answer 8

Remember that enums in C# have the potential to bite you:

enum SwitchPosition 
{
    Off = 0,
    On = 1
}

void SetLightStatus(SwitchPosition pos)
{
    switch (pos)
    {
        case On: 
            TurnLightOn(); 
            break;
        case Off: 
            TurnLightOff(); 
            break;
        default:
            // Fugedaboudit -- will never happen, right?
    }
}

Wrong! Calling SetLightPosition(2); is legal and will fall through the cases in that switch statement. Best to throw a HorrifyingSituationError as suggested by S. Lott.

Answer 9

Personally, I would write the else line as:

} else /*if (person.IsFemale())*/ {

This sidesteps having to run the function (I'd hate to waste the time running it if its results aren't needed) but leaves important documentation for future developers. It is now clear that this branch of the conditional covers the IsFemale case (specifically), not the !IsMale case (in general). You are essentially making your assumptions "out loud", which makes it less likely that future changes will mis-understand what you are doing and break your code.

In our embedded systems, it can be difficult to analyze and debug failures so we often include the "impossible" else statements and make them spit out error messages and throw an exception. This is usually restricted to development builds, and once the code is stable they are removed.

Answer 10

If you want to indicate a block of code that "can't happen", use

assert (false);

Answer 11

If it CAN'T happen in production (but might happen during development), I use assert:

If it SHOULDN'T happen in production, but it possibly could, I either return an error or throw an exception.

By the way, here's a neat little C++ trick I picked up somewhere; since many ASSERT macros will display their expression in a message box if the assertion is untrue, you can use the following form for branches that should never get executed:

if (person.IsMale())
{
    AdmitEntranceToManCave();
}
else
{
    ASSERT(!"A female has gotten past our defenses. EVERYBODY PANIC!");
}

The string literal evaluates to a (non-NULL) address, which is logically TRUE. Thus, using the logical NOT operator makes it FALSE.

Answer 12

The question depends on your context -- is anyone else ever going to be able to extend your Person object? When androids become legal People, you might find that both isMale() and isFemale() can be false.

This is especially relevant if the code you're writing is a module that's going to be used by people outside your group. Of course, in that case you could go a step further, skip the hard-coded if tests, and call doGenderStuff() on an object created by the appropriate factory...

Answer 13

Since there are so many answers, I'll just show you what overdoing it looks like: (in C++)

if(!DoOperation())
{
    // Allocate for a message
    char * pMsg = new char[256];
    // Make sure the memory was allocated
    if(!pMsg)
    {
        cout << PREDEFINED_OUT_OF_MEMORY_MESSAGE << endl;
        exit(0);
    }

    if(!strcpy(pMsg,"Operation failed!"))
    {
        cout << PREDEFINED_STRCPY_FAILED_MESSAGE << endl;
        exit(0);
    }

    // Now let the user know there was an error
    ErrorDetailStructure * pError = new ErrorDetailStructure();
    if(!pError)
    {
        cout << PREDEFINED_OUT_OF_MEMORY_MESSAGE << endl;
        exit(0);
    }

    // Copy the message to the error structure
    if(!strcpy(pError->pMessage,pMsg))
    {
        cout << PREDEFINED_STRCPY_FAILED_MESSAGE << endl;
        exit(0);
    }

    // Alert the user - yes, ErrorDetailsStructure
    // overloads operator<<
    cout << pError;

    delete pError;  // the destructor frees pMessage member

    // Now we need to log this error
    some_file_pointer * pFile = OpenAFilePointer("/root/logfile");

    if(!some_file_pointer)
    {
        cout << PREDEFINED_OPEN_FILE_ERROR_MESSAGE << endl;
        exit(0);
    }

    some_file_pointer->WriteError("Something went wrong. Guess what it was.");

    // Don't forget to free some_file_pointer
    delete some_file_pointer;

    exit(0);  // Just quit. Give up.
}

I ended up having a lot of fun with this. There are so many problems with this (and the design is so bad) that I got a good laugh writing it.

Answer 14

I would keep the code as clean and short as possible, and then add assertions to places that don't contaminate the cleanliness :)

if(male)
{
  ...
}
else
{
  ...
}

// other stuff that nobody cares about...
assert(male||female);