views:

368

answers:

5

I'm not sure if this is a duplicate but if it is please feel free to close this.

Background: I have a timer component for a game i'm writing that supports Stop and Pause methods. The non-critical error cases in question are doing things like calling Pause when in the timer is already paused, this is not fatal but it's not going to cause the change in state implied by the method name

Questions:
1. How does one generally indicate a non-fatal but abnormal condition?
2. Should I actually be throwing an exception in these cases? I think it's a little heavy-handed considering calling Pause when paused wont do any harm
3. Am I over-thinking this

UPDATE:
Based on the responses and comments here is the statergy I have chosen to take: In development builds an exception will occur because I consider these bugs and I'd like to catch them and correct them. I can't justify an exception in a release build because these bugs don't corrupt the game state, and I don't think users of the application would appreciate the loss of their hard earned score because I failed to code properly

Thanks for all your responses, this has been very educational for me.

+7  A: 

I think your fine to just ignore the call if the timer is already paused. Assuming the method is called Pause() then the client only requires that after the method call that the timer is paused, and exceptions should only be thrown if it won't be; the method doesn't imply any requirement on the current state of the timer.

On the other hand if the method was called ChangeTimerFromRunningToPaused() then that implies that it is only valid to call this on a running timer. Under these circumstances I would expect it to throw an exception informing the client that the timer is not in a valid state to process this method call.

Martin Harris
+1  A: 

In such cases (moving some state machine to a state on which it already is), I'd just skip the call. Usually, non-fatal errors or warnings should not be thrown as exception, but rather returned as result (which can be ignored or processed, whatever fits the situation best).

Lucero
+1  A: 

No, don't throw an exception, Exceptions are heavy. There's nothing wrong to pause again. I don't see any harm.

Vadim
+3  A: 

Against the other answers, I would consider throwing an exception. If it would be my code performing the second Pause() call, I might consider my code to be broken if it makes this call where it should not. Maybe I am missing a check like if (Timer.IsRunning) { } and I don't want to hide this bug by silently ignoring the call. Even if the call is triggered by user input - a button click for example - this might be an indication of bad design and I should have a look at it. Maybe the button should be disabled.

The more I think about it, the less situations come to mind where the second call should not be an exception indicating bad calling code. So, yes, I would throw the exception.

UPDATE

In a comment Martin Harris suggest using Debug.Assert() to get the bug during development. I think that this is to weak because you won't get the bug in production code. But of course you don't want to crash an application in production because of this non-fatal error. So we will just catch this exception up the stack and generate a error report and than resume as if nothing happend.

Here are two goo links.

API Design Myth: Exceptions are for "Exceptional Errors"
Coding Horror - Exception-Driven Development

Daniel Brückner
In cases like this I'd use a Debug.Assert(Timer.IsRunning). Since I would argue that since call isn't fatal (and there for not exceptional) but may not be optimal and it could be helpful to know about it while you are debugging.
Martin Harris
I would go far as to add an overload to allow the caller to explicitly specify whether it matters. Totally agree that it is likely a sign of something wrong. But like most signs there are exceptions... like trying to avoid bloat with hypersensitive race condition contingency planning.
Fowl
Having a overloaded version might be good in a few situations. But if I would have to use the overload regulary I would start thinking about refactoring it to PauseTimerIfRunning() and moving the check into the method. I think Debug.Assert() is to weak because it won't get the bug if the code is in production. But I will update the answer to clearify that.
Daniel Brückner
It's a more interesting problem than I originally thought. I guess the answer is "what would the client of your code expect to happen?" If I was the client then I would expect that the call would just be ignored - after all I'm only interested in it being paused at the end of the call. Daniel would expect it to throw an exception - after all it didn't actually perform the action he expected, and Svish (in the question comments) expects it to restart - after all that's the way his tapedeck works. So if the code isn't a public library just do what you'd expect. +1 for making me think.
Martin Harris
A: 

This looks similar (though not identical) to one of my questions, perhaps the answers there are helpful for you.

Daan