Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have an enumeration with the commands Play, Stop and Pause for a media player. In two classes I do a switch-case over the received commands. The player runs in a different thread and I deliver the commands in a command queue to the thread.

If I generate class diagrams the enumeration has dependencies all over the place. Is there a nicer way to deal with the commands? If I would change/extend the enumeration, I would have to change several classes. (Its not super important to keep the player extensible, but I try to write nice software.)

share|improve this question
    
I just want to make a comment that though @MainMa's answer makes sense in this situation, using an enum in a swtich statement in general is not bad practice and can make your code much more readable. It's just a case of getting a feel for when it's appropriate. public enums should probably be saved for things like configuration values of a class, etc. –  TZHX Sep 22 '12 at 14:28
    
@TZHX - in some cases, sure, they make sense. In general though, switch statements are an abused flow control mechanism; often, they indicate a single method where there should have been multiple. As with this case, there really isn't any reason to squeeze these 3 methods into one, and things often get fun quickly with different parameters for the various cases in the switch. I would recommend only using them when you want to unify multiple methods for some reason - e.g. having a single validate method for a form, which then delegates out, based on state. –  Daniel B Sep 22 '12 at 15:40
add comment

2 Answers

up vote 4 down vote accepted

A "nicer" way is to have three methods, one per command.

There is no need to collapse all those commands into one method and to use a switch later. Since those commands do different things, they deserve their own methods in the interface.

Instead of:

public void ChangeState(PlayerState newState)
{
    switch (newState)
    {
        case PlayerState.Play:
            // Start playing.

        case PlayerState.Stop:
            // Stop playing.

        case PlayerState.Pause:
            // Pause or resume.
    }
}

you would have:

public void Play()
{
    // Start playing.
}

public void Stop()
{
    // Stop playing.
}

public void Pause()
{
    // Pause or resume.
}

Why?

Your current implementation using a switch will:

  • either do multiple things,
  • or will just serve to call Play, Stop and Pause methods.

In the first case, you break the rule which says that a method should do one and one only thing.

In the second case, KISS: don't write a method you really don't need and which doesn't bring anything useful to the API.

share|improve this answer
add comment

What you seem to be doing is taking 3 separate inputs, merging them into a single variable, and passing that along to be processed. Your processing code then does a switch / if statements on this variable.

As MainMa mentioned, the obvious and cleaner approach would be to simply have 3 methods, and have your interface call those. If, for some reason, you wish to have a slightly more disconnected model (say, if you need to execute the command at a later stage then when it's triggered), the typical way is the Command Pattern where you effectively encapsulate a method call in an object (which can be serialised / stored / sent over the network / run multiple times, etc). The advantage over your enum is that it caters for input parameters (e.g. if you have a volume control command). That said, this is an unneccessary over-complication, and I'd stick to plain old methods.

share|improve this answer
    
I would like to use the different methods, but I have to call them somehow in the new thread. Would the command pattern be justified in this case? (As far as I know, I should not call methods on runnables from the main thread) –  Puckl Sep 22 '12 at 13:13
    
@Puckl In terms of not calling methods from the main thread, it really depends on whether they are long-running operations, or not - if it's likely that the operation will be too quick for the user to notice, running it in a background thread is generally just complicating the code. Aside from that, it depends on how you intend to handle the different threads. If you have a worker thread that processes incoming requests, the command pattern can work, and may be justified. In C#, however (I'm guessing your language), there are easier ways to make async calls. –  Daniel B Sep 22 '12 at 15:30
    
The Command Pattern is basically a workaround for OOP languages that lack first-class functions and/or closures. In a sufficiently equipped language, one can just pass the relevant command functions directly. In C#, I'd say delegates are the way to go. –  tdammers Sep 22 '12 at 18:41
    
@tdammers Yepp, delegates / closures for C# would be best. Re: command, the "workaround" definition only really holds if you only use it for that, in which case I agree. I'm yet to see a language give first-class support for the things I normally do with Commands, like serialising them for replayability / audit trails, etc. I'm not sure I want a language which goes that far. –  Daniel B Sep 22 '12 at 21:02
    
@DanielB: Good point on the replay etc.; I don't think it's relevant here, since you won't get those features with switch/enum either. –  tdammers Sep 23 '12 at 14:21
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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