16

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?

closed as primarily opinion-based by gnat, maple_shaft May 10 '16 at 12:29

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 21
    Don't use exceptions to model behavior that isn't exceptional. – Alexander May 9 '16 at 16:33
  • 1
    Hey! I don't think it's a duplicate. My question is more about IllegalStateException and the above question is about using exceptions in general. – x2bool May 9 '16 at 19:30
  • 1
    Instead of JUST silently failing, why not ALSO log the anomaly as a warning? Surely the Java you're using supports different log levels (ERROR, WARN, INFO, DEBUG). – Eric Seastrand May 9 '16 at 20:10
  • 3
    Notice that Set.add does not throw an exception if the item is already in the set. Its purpose is to "ensure the element is in the set", rather than "add the item to the set", so it achieves its purpose by doing nothing if the element is already in the set. – immibis May 9 '16 at 20:56
  • 2
    Word of the day: idempotence – Jules May 10 '16 at 10:49

11 Answers 11

37

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.

  • 15
    This answer reminds us that sometimes it's a good idea to sketch out the state transitions for even simple interactions. – user156792 May 9 '16 at 15:01
  • 6
    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. > Exceptions aren't designed to punish you: They're designed to tell you something unexpected has happened. I personally would be very thankful for a MediaPlayer.stop() method which threw an IllegalStateException instead of spending hours of debugging code and wondering why the Hell "nothing happens" (i.e. "it just doesn't work"). – errantlinguist May 9 '16 at 16:26
  • 3
    Of course, if your design says the loopback transition is invalid then YES you should throw an exception. However, my answer was about a design where that transition is perfectly OK. – MetaFight May 9 '16 at 17:24
  • 1
    StopOrThrow() sounds horrible. If you're going down that alley why not use the standard pattern of TryStop()? Also, generally, when the behaviour of an API is unclear (as most of them are) developers aren't expectted to simply guess or experiment, they're expected to look at the documentation. That's why I like MSDN so much. – MetaFight May 10 '16 at 0:42
  • 1
    Personnally i prefer the fail-fast option, so i would go for IllegalStateException in order to indicate to the API's user that there is something wrong in his logic, even if it's harmless. I often get myself those exceptions from my own code, really helpfull to detect that my unfisnihed work is already wrong – Walfrat May 10 '16 at 9:57
17

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.

  • 2
    +1 for the note about returning the old state: That allows implementing it in a way which makes the whole operation (querying the status and transitioning to a defined status) atomical, which is at the very least a nice feature to have. It would make it easier to write robust user code that does not fail sporadically due to some weird race condition. – cmaster May 9 '16 at 19:36
  • A bool TryStop() and a void Stop() code example would make this a truly excellent answer. – RubberDuck May 9 '16 at 22:32
  • 2
    @RubberDuck: That wouldn't be a good pattern. The name TryStop would imply that an exception should not be thrown if the player can't be forced to a stopped state. Trying to stop the player when it's already stopped is not an exceptional condition, but is a condition a caller may be interested in. – supercat May 9 '16 at 22:37
  • 1
    Then I've misunderstood your answer @supercat – RubberDuck May 9 '16 at 22:38
  • 2
    I'd like to point out that testing and setting this way is NOT a violation of the law of demeter provided the API also provides a way to test playback state without changing it. This could be a completely different method, say isPlaying(). – candied_orange May 9 '16 at 23:41
11

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.

11

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.

  • +1. This is the first answer that shows why an exception isn't appropriate here. Exceptions indicate that the caller is likely to need to know about the exceptional case. In this case, almost certainly the client of the class in question won't care whether calling 'stop()' actually caused a state transition, so would likely not want to know about an exception. – Jules May 10 '16 at 10:45
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.

3

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.

3

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.

2

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.

1

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.

  • It's not supposed to be called because calling it indicates a bug in the caller. – immibis May 9 '16 at 20:57
  • @immibis: Not necessarily. For instance, that caller could be an on-click listener of some UI button. (As a user, you probably wouldn't like an error message if you happen to accidentally click the stop button if the media is already stopped.) – meriton May 9 '16 at 23:14
  • @meriton If it was the on-click listener of a UI button then the answer would be obvious - "do whatever you want to happen if the button is pressed". Clearly it's not, it's something internal to the application. – immibis May 9 '16 at 23:24
  • @immibis - I edited my response. I hope it helps. – Jim May 10 '16 at 4:42
1

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.

0

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.

  • 3
    I disagree about playToggle() being easier to use: To achieve the desired effect (player playing or not playing), you would be required to know its current state. So, if you get user input requesting you to stop the player, you would first need to inquire whether the player is currently playing, then toggle its state or not depending on the answer. That's much more cumbersome than just calling a stop() method and be done with it. – cmaster May 9 '16 at 19:29
  • Well I've never said it was easier to use - my claim was that it's safer. Plus, your example assumes that the user would be given separate stop and play buttons. If that were indeed the case then yes, a playToggle would be very cumbersome to use. But if the user was also given a toggle button instead, then playToggle would be easier to use than play/stop. – Pedro Rodrigues May 9 '16 at 22:28

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