What is better IllegalStateException or silent method execution?

Question

Let's say I have a MediaPlayer class which has play() and stop() methods. What is the best strategy to use when implementing the stop method in case when the play method has not been called before. I see two options: throw an exception because the player is not in appropriate state, or silently ignore calls to the stop method.

What should be the general rule when a method is not supposed to be called in some situation, but it's execution does no harm to the program in general?

Answer 1

There is no rule. It's entirely up to how you want to make your API "feel."

Personally, in a music player, I think a transition from the state Stopped to Stopped by means of the method Stop() is a perfectly valid state transition. It's not very meaningful, but it is valid. With this in mind, throwing an exception would seem pedantic and unfair. It would make the API feel like the social equivalent of talking to the annoying kid on the school bus. You're stuck with the annoying situation, but you can deal with it.

A more "sociable" approach is to acknowledge that the transition is at worst harmless and be kind to your consuming developers by allowing it.

So, if it were up to me, I'd skip the exception.

Answer 2

There are two distinct kinds of actions one may wish to perform:

1. Simultaneously test that something is in one state, and change it to another.

2. Set something to a particular state, without regard for the previous state.

Some contexts require one action, and some require the other. If a media player which reaches the end of content will remain in a "playback" state, but with the position frozen at the end, then a method which simultaneously asserts that the player was in a playback state while setting it to a "stop" state might be useful if code would want to ensure that nothing had caused playback to fail prior to the stop request (which might be important if e.g. something was recording the content being played back as a means of converting the media from one format to another). In most cases, however, what matters is that after the operation the media player is in the expected state (i.e. stopped).

If a media player automatically stops itself when it reaches the end of the media, a function which asserts that the player was running would likely be more annoying than helpful, but in some cases knowing whether the player was running at the time it was stopped could be useful. Perhaps the best approach to cater to both needs would be to have the function return a value indicating the player's previous state. Code which cares about that state could examine the return value; code which doesn't care could simply ignore it.

Answer 3

There is no general rule. In this specific case, the intention of the user of your API is to stop the player from playing the media. If the player is not playing the media, the MediaPlayer.stop() may do nothing and the goal of the method caller will still be achieved - the media is not playing.

Throwing an exception would require the user of the API to either check if the player is currently playing or catch and handle the exception. This would make the API more of a chore to use.

Answer 4

The purpose of exceptions isn't to signal that something bad has happened; it's to signal that

1. Something bad has happened
2. that I don't know how to fix here
3. that the caller, or something up the callstack from it should know how to deal with
4. so I'm bailing out quickly, halting execution of the current code path in order to prevent damage or data corruption, and leaving it to the caller to clean up

If the caller tries to Stop a MediaPlayer that's already in a stopped state, this isn't a problem that the MediaPlayer can't resolve (it can simply do nothing.) It isn't a problem that, if continued, will lead to damage or data corruption, since it can be successful simply by doing nothing. And it isn't really a problem that it's more reasonable to expect the caller to be able to resolve than the MediaPlayer.

Therefore, you shouldn't throw an exception in this situation.

Answer 5

Look at it this way:

If the client calls Stop() when the player isn't playing, then Stop() is automatically successful because the player is currently in the stopped state.

Answer 6

The rule is you do what the contract of your method requires.

I can see multiple way to define meaningful contracts for such a stop method. It could be perfectly valid for the stop method to do absolutely nothing if the player isn't playing. In that case, you define your API by its goals. You want to transition to the stopped state, and the stop method does that - just by transitioning from the stopped state into itself without error.

Is there any reason why you would want to throw an Exception? If the user code will look like this in the end:

try {
    player.stop();
} catch(IllegalStateException e) {
    // do nothing, player was already stopped
}

then there is no benefit to having that exception. So the question is, will the user care about the fact that the player was already stopped? Does he have another wa to query it?

The player might be observable, and might already notify observers about certain things - e.g. notifying observers when it transitons from playing to stopping to stopped. In this case, having the method throw an exception is not necessary. Calling stop will just do nothing if the player is already stopped, and calling it when its not stopped will notify the observers about the transition.

all in all, it depends on where your logic will end up. But I think there are better design choices then throwing an exception.

You should throw an exception if the caller has to intervene. In this case he doesn't have to, you can simply let the player be as it is and it continues to work.

Answer 7

There is a simple general strategy that helps you make this decision.

Consider how the exception would be handled if it was to be thrown.

So imagine your music player, user clicks stop and then stop again. Would you want to display an error message in that scenario? I haven't yet seen a player that does. Do you want the application behaviour to be any different than just clicking stop once (like log the event or send an error report in the background)? Probably not.

That means the exception would need to be caught and swallowed somewhere. And that means you're better off not throwing the exception in the first place because you don't want to take a different action.

This doesn't just apply to exceptions but any kind of branching: basically if there needs to be no observable difference in behaviour, there is no need for branching.

That said, there isn't much harm in defining your stop() method to return a boolean or an enum value indicating whether stopping was "successful". You probably won't ever use it, but a return value can be more easily and naturally ignored than an exception.

Answer 8

Here is how android does it: MediaPlayer

In a nutshell, stop when start has not been called is not a problem, the system stays in stopped state, but if called on a player that does not even know what it is playing, an exception is thrown, because there is no good reason for calling stop there.

Answer 9

What exactly do MediaPlayer.play() and MediaPlayer.stop() do?-- are they event listeners for user input or are they actually methods which start some sort of media stream on the system? If all they are are listeners for user input, then it's perfectly reasonable for them to do nothing (although it would be a good idea to at least log them somewhere). However, if they affect the model your UI is controlling, then they could throw an IllegalStateException because the player should have some sort of isPlaying=true or isPlaying=false state (it needn't be an actual Boolean flag like written here), and so when you call MediaPlayer.stop() when isPlaying=false, the method can't actually "stop" the MediaPlayer because the object isn't in the appropriate state to be stopped -- see the description of the java.lang.IllegalStateException class:

Signals that a method has been invoked at an illegal or inappropriate time. In other words, the Java environment or Java application is not in an appropriate state for the requested operation.

Why throwing exceptions can be good

A lot of people seem to think that exceptions are bad in general, as in they should never be thrown (cf. Joel on Software). However, say you have a MediaPlayerGUI.notifyPlayButton() which calls MediaPlayer.play(), and MediaPlayer.play() invokes a bunch of other code and somewhere down the line it interfaces with e.g. PulseAudio, but I (the developer) don't know where because I didn't write all that code.

Then, one day while working on the code, I click "play" and nothing happens. If MediaPlayerGUIController.notifyPlayButton() logs something, at least I can look into the logs and check that the button click was in fact registered... but why doesn't it play? Well, say there's in fact something wrong with the PulseAudio wrappers which MediaPlayer.play() invokes. I click the "play" button again, and this time I get an IllegalStateException:

 Exception in thread "main" java.lang.IllegalStateException: MediaPlayer already in play state.

     at org.superdupermediaplayer.MediaPlayer.play(MediaPlayer.java:16)
     at org.superdupermediaplayer.gui.MediaPlayerGUIController.notifyPlayButton(MediaPlayerGUIController.java:25)
     at org.superdupermediaplayer.gui.MediaPlayerGUI.main(MediaPlayerGUI.java:14)

By looking at that stack trace, I can ignore everything before MediaPlayer.play() when debugging and spend my time figuring out why e.g. no message is being passed to PulseAudio to start an audio stream.

On the other hand, if you don't throw an exception, I'll have nothing to start working with other than: "The stupid program doesn't play any music"; Although not as bad as explicit error hiding such as actually swallowing an exception which was in fact thrown, you're still potentially wasting others' time when something does go wrong... so be nice and throw an exception at them.

Answer 10

What should be the general rule when a method is not supposed to be called in some situation, but it's execution does no harm to the program in general?

I see what could be a minor contradiction in your statement that may make the answer clear to you. Why is it that the method is not supposed to be called and yet it's execution does no harm?

Why is the method "not supposed to be called"? That seems like a self-imposed restriction. If there is no "harm" or impact to your API, then there is not an exception. If the method really should not be called because it may create invalid or unpredictable states, then an exception should be thrown.

For example if I walked up to a DVD player and hit "stop" before hitting "start" and it crashed, that would not make sense to me. It should just sit there or, worse case, acknowledge "stop" on the screen. An error would be annoying and not helpful in any way.

However, can I put in an alarm code to turn it "off" when it is already off (calling the method), that may cause it to enter a state that would prevent it from properly arming it later? If so, then throw an error even though, technically, "there was no harm done" because there may be problem later. I would want to know about this even if it didn't actually go into that state. Just the possibility is enough. This would be an IllegalStateException.

In your case, if your state machine can handle the invalid/unnecessary call then ignore it, otherwise throw an error.

EDIT:

Note that a compiler does not invoke an error if you set a variable twice without inspecting the value. It also doesn't prevent you from re-initializing a variable. There are many actions that could be considered a "bug" from one perspective, but are merely "inefficient", "unnecessary", "pointless", etc. I tried to provide that perspective in my answer - doing these things is not generally considered a "bug" because it does not result in anything unexpected. You probably should approach your problem similarly.

Answer 11

I prefer designing the API in a way that makes it harder or impossible for the consumer to make mistakes. For example, instead of having MediaPlayer.play() and MediaPlayer.stop(), you could provide MediaPlayer.playToggle() which toggles between "stopped" and "playing". This way, the method is always safe to call - there's no risk of going into an illegal state.

Of course, this is not always possible or easy to do. The example you gave is comparable to trying remove an element that was already removed from the list. You can either be

• pragmatic about it and not throw any error. This certainly simplifies a lot of code, e.g., instead of having to write if (list.contains(x)) { list.remove(x) } you only need list.remove(x)). But, it can also hide bugs.
• or you can be pedantic and signal an error that the list doesn't contain that element. The code can become more complicated at times, as you constantly have to verify if the pre-conditions are satisfied before calling the method, but the upside is that eventual bugs are easier to track down.

If calling MediaPlayer.stop() when it's already stopped has no harm in your application, then I'd let it run silently because it simplifies code and I have a thing for idempotent methods. But if you're absolutely certain that MediaPlayer.stop() would never be called in those conditions, then I'd throw an error because there might an bug somewhere else in the code and the exception would help track it down.