views:

163

answers:

7

Say you have the following code block:

if (Light.On) {
    // do something...
} else if (Light.Off) {
    // do something else...
} else {
    // this state should never be reached
}

Now assume that the applications logic dictates that in this part of the code, the last state should never be reached, but it is not something that can be determined at compile time. And maybe there ARE other states (e.g. Light.Broken) which could be set by other parts of the application, but which are not used here.

What code do you add in the last else block?

  1. Add no code, because it should not be reached anyway.
  2. Add some logging functionality, so that you as a developer know that some illegal state has been reached.
  3. Throw an exception, because the state must not be reached and if it is reached anyway, something else must be wrong.

The first option doesn't seem reasonable to me, hoping something goes right seems hardly the right choice. Option two has the advantage that your app doesn't crash right away, so if this happens in a rare occasion which was not caught in testing, the customer can continu using the application and the developer is notified of the problem. Option three causes the app to crash, which obviously is not something you want your customers to experience, but it does make it very clear that something is wrong.

What is the best way to handle such a situation?

EDIT, based on comments:

Some additional consideration to steer the discussion:

  • The contract of the method that contains the above code does not allow any other values to be set at that moment then On and Off.
  • Assume that the code is in a not-so-critical part of the application.
A: 

3rd option is the correct as if for that part any other state is not valid so it should throw and exception saying invalid state.

Bhushan
+6  A: 

In development - fail hard and fail fast. Throw some kind of runtime exception, or just Assert(false).

In a release - shutdown gracefully. Your app is in an unusable state, and you cant realy on anything that you normally would, such as class invariants etc. Give the user a chance to save thir work, for example, try and log an error that could be sent back to the dev team and then shutdown.

EDIT: Based on comments added.

If the contract of the function states that the light shouold be on or off at function entry then any other state is an error. The function should fail, according to the principles outlined in my original answer.

Regarding the 'non-critical' aspaect - if a function precondition is not being met that means that your application is broken. Regardless of wether the error is detected in a non-critical piece of code, that does not imply that the problem itself is non-critical - you have no way of knowing that the bug that creates the invalid state doesnt also affect critical areas of code.

Visage
How do you implement such a solution? Do you actually implement some 'development' flag and base your error handling on its value? It seems to me like that wouldn't make the code very clean...
Daan
Our build script prompts for a version number. If one is provided a reelase build is done, if it isnt the version is marked as an engineering release. At runtime we have a standard error reporting function that behaves accordingly.
Visage
Since Java 1.4 you can also use the assert instruction, which generates an AssertionError. By default, assertions are ignored at runtime (behave as no-ops) but you can enable them with -ea JVM switch.
Bartosz Klimek
+2  A: 

Well ... It depends. Having a third case for a boolean test would make me want to cry. It's just noise, adding confusion and telling me the developer was at least as confused as it makes me feel.

For a non-boolean, I guess you could do whatever ... If the ifs capture the states that are relevant to the code in question, there's no harm in just ignoring other cases. If there's a good chance that further cases will be needed in the future, a comment might be enough to indicate that for clarity.

unwind
In this case, the if-statements do capture the states that are relevant to the code in question, but other states are NOT ALLOWED here. The question is how to handle the (illegal) situation where a not-allowed state is reached. Would you simply ignore that case in this situation too?
Daan
Don't ignore anything that's not allowed. There's a difference between "not allowed" and "not important".
paxdiablo
@Pax I agree, but then the question still remains: make a log entry and continu, or immediately shut down the application, even though this check may be in some not-so-critical part of the app?
Daan
Not-critical = continue, critical = stop. Criticality depends on the app.
paxdiablo
It depends on the function's contract. If it demands that the light is either on or off at entry then other cases are a violation of that contract, and so the function should fail. If the function doesnt have such an explicit constraint then it is the functions responsibility to handle other cases
Visage
A: 

Don't do anything. If all you care about is whether the light is on or not:

if (Light.On) {
    // do some work
} else {
    // too darned dark to work
}

Otherwise you should enumerate all your states.

If the other states are not allowed, you should error. If they're allowed but irrelevant, just ignore them. With properly designed code, there is no problem.

You're main problem here lies in a design that can have Light.On and Light.Off set at the same time. You should be using a state Light.State which is set to one of {on, flickering, off, broken, unplugged, exploded, emitting_dangerous_gamma_rays} and so on.

paxdiablo
I only care if the light is On or Off, but I also KNOW that the light SHOULD only be On or Off at this point. If it is something else, some part of my code must be broken. Do you still simply ignore the situation then?
Daan
Light in this code example could be some enumeration, so On and Off cannot be set at the same time. You can think of On, Off, ... as states in that regard.
Daan
That means critical (your code is broken). Save what you can (somewhere other than in the users document since a broken program means your data can't be trusted) and exit immediately with an error.
paxdiablo
A: 

Throw an exception.

As you said, the third case should never occur. If it does happens, then something went wrong, and you don't know what else can be going wrong.

You also don't want calling code to think that code is working normally, when it is actually failing.

Additionally it makes it a lot easier to spot the issue Before it is released.

eglasius
+1  A: 

As already said, in development phase you should make it visible as soon as possible. In release phase, it depends on how critical it is to reach this invalid state.

The least is to issue a log for debugging purpose.

Then you can try to recover from this invalid state either by going back to a previous valid state or by going to a new valid state.

Finally if nothing safe can be done, you can terminate the execution (with an alert to the user and a log to the maintainer).

mouviciel
A: 

In the development version, just throw an exception.

In the release version:

  • Throw an exception
  • Save the user's work in some temporary file if possible
  • Save a crashdump and (using a small helper application) offer the user the option of sending it to you so you can identify and fix the problem
Adrian Grigore