6
\$\begingroup\$

I have a very long switch statement which is expected to have nearly 50-55 cases.

switch (commandType) {
            case COMMAND_LOGIN:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_MAKE_READY:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_MAKE_NOT_READY:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_MAKE_NOT_READY_WITH_REASON:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_LOGOUT_WITH_REASON:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_ANSWER_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_HOLD_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_RETRIEVE_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_END_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_TRASFER_SST_CALL:
                command = getCommand(commandType, agentId);
                break;

            case COMMAND_CONSULT_CALL:
                command = getCommand(commandType, agentId);
                break;
                 .
                 .
                 .
                 .
                 .
           default :
                command = "";
                break;

        }//end switch

It doesn't seem to be something which should exist in this current form, I have gone through some questions here 'but they doesn't seem to help in this condition' I have seen the "strategy pattern", but that also seems to be suitable for the case where i have to execute different method for every case. Kindly guide me how the size of this code can be reduced.

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to CodeReview.SE. I might be missing something but can't this be simply written : command = getCommand(commandType, agentId);. Also, this place is about reviewing real code, not some kind of idea. \$\endgroup\$ – SylvainD Dec 24 '14 at 8:21
  • \$\begingroup\$ thankyou josay, this is my real code, and yes i have got your point. \$\endgroup\$ – pointer Dec 24 '14 at 8:22
  • \$\begingroup\$ that would be the default case, i have just edited the original post. Now there will be a value of "commandType" for which the case will not exist, so if the switch is removed and only command = getCommand(commandType, agentId); is used, a command for an invalid value of "commandType" will also be constructed. \$\endgroup\$ – pointer Dec 24 '14 at 8:31
  • \$\begingroup\$ This is example code right now, or (sorry to say) very bad production code, I would expect every command to be calculated differently depending on which case you are in. \$\endgroup\$ – skiwi Dec 24 '14 at 8:38
  • \$\begingroup\$ yes i know this is very bad' that's why i need your guidance to make it at-least ok \$\endgroup\$ – pointer Dec 24 '14 at 8:39
8
\$\begingroup\$

Use Enum:

public enum CommandType{
        COMMAND_HOLD_CALL(0), 
        COMMAND_RETRIEVE_CALL(1), 
        .....
        COMMAND_RETRIEVE_CALL(55),
        COMMAND_MAX(56),
        ;
        private int commandId;

        private CommandType(int commandId){
            this.commandId= commandId;
        }      
        public int getValue(){
            return this.commandId;
        }
    }
}

So your switch statement will reduce to:

if(commandType.getValue()<CommandType.COMMAND_MAX.getValue()){ // The range is from  0 to 55
      command = getCommand(commandType, agentId);
}else{
      command = "";
}

Any new addition or remove will affect the enum class and nothing else.

\$\endgroup\$
  • \$\begingroup\$ How do you propose map the incoming integer commandType to its dual in your enumeration? It appears to me that's where the 'real' validation takes place. \$\endgroup\$ – user59064 Dec 24 '14 at 12:38
  • \$\begingroup\$ Is the commandType user input or calculated? You could share the code of commandType and getCommand to help simplify and provide a better solution. \$\endgroup\$ – thepace Dec 24 '14 at 15:47
  • \$\begingroup\$ Is commandType a string or an integer? \$\endgroup\$ – thepace Dec 24 '14 at 15:50
  • \$\begingroup\$ commandType is a string and it is a user input, so now we need to check weather this string exists in enum list or not, for that we can use the valueOf() method for enum. \$\endgroup\$ – pointer Dec 24 '14 at 19:44
10
\$\begingroup\$

Giant switch statements are a code smell. In Java, the usual remedy is to refactor using the Replace Conditional with Polymorphism recipe.

However, I think your problem runs even deeper. I raise the following objections:

  • Why should this code exist at all? Shouldn't getCommand(commandType, agentId); just return the appropriate result for all values of commandType? Or are there specific command types that you want to disable? The latter possibility seems disturbing. If this code is, say, part of a permission-checking system to allow only certain types of commands, I would say that the permission-checking system is woefully underdeveloped.
  • Why does getCommand(…) return a String? What use could a constant-to-string translation possibly have?

I wish I could give you more constructive suggestions, but unfortunately, you haven't presented much context in the question to allow a deeper analysis.

\$\endgroup\$
6
\$\begingroup\$

You could use fallthrough in order to remove the amount of duplicated code.

    switch (commandType) {
        case WHATEV_1:
        case WHATEV_2:
        case WHATEV_3:
        case WHATEV_4:
        case WHATEV_5:
        case WHATEV_6:
        case WHATEV_7:
        case WHATEV_8:
        case WHATEV_9:
            command = getCommand(commandType, agentId);
            break;
        default :
            command = "";
            break;
    }//end switch

Now, different situations are possible :

  • you have an enum where each value is properly handled and you have this default just in case something gets added to the enum : this can be detected at compilation time.

  • if you have an enum where a handful of known values will lead to command = "" : it might be better to write these cases explicitely and default to command = getCommand(commandType, agentId);.

  • if you don't have an enum, maybe you should.

In any case, maybe the switch case could be part of the logic in the getCommand (and return ""). It only moves the issue more that it solves it but I'd expect a switch to be hidden somewhere in that method.

\$\endgroup\$
  • \$\begingroup\$ thanx josay for elaborate explanation. \$\endgroup\$ – pointer Dec 24 '14 at 9:18
1
\$\begingroup\$

In your code example, all but the default case have the line command = getCommand(commandType, agentId);

You could create a container list or map of all the possible cases, and if commandType does not exist in the container, set the command to an empty string. Otherwise, use the line above to set the command.

\$\endgroup\$
1
\$\begingroup\$

@Josay's answer is the best. (I'm just reinforcing it.)

When COMMAND_LOGIN etc. are all compile-time constants, decent compilers will generate a branch table, and execution will be extremely fast. See http://en.wikipedia.org/wiki/Branch_table

I think it is a bad idea to make code more complicated in order to make it shorter. If someone else needs to maintain your code in a few months, they will have an easy time eyeballing your 200-line switch statement, without having to think too hard. The only thing I would improve would be to group commands that use the same getCommand() variant, as Josay describes, so the code looks like this:

switch (commandType) {
    case COMMAND_LOGIN:                 // status-change
    case COMMAND_MAKE_READY:
    case COMMAND_MAKE_NOT_READY:
    case COMMAND_MAKE_NOT_READY_WITH_REASON:
    case COMMAND_LOGOUT_WITH_REASON:
        command = getStatusCommand(commandType, agentId);
        break;

    case COMMAND_ANSWER_CALL:           // call-handling
    case COMMAND_HOLD_CALL:
    case COMMAND_RETRIEVE_CALL:
    case COMMAND_END_CALL:
    case COMMAND_CONSULT_CALL:
        command = getCallCommand(commandType, agentId);
        break;

    case COMMAND_TRASFER_SST_CALL:      // transfer
        command = getTransferCommand(commandType, agentId);
        break;
         . . .
    default:
        command = "";
        break;
}//end switch
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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