1

I'm refactoring code and have reached a horribly gigantic switch statement. Every single API method available to end users is represented as an enum and we have a switch statement iterating over the enum and responding to each possible API call.

Each individual enum option has only a few lines of code. Any sufficiently complicated command gets a method call that is made. However, with so many different enums ever a half dozen lines per enum grows to be rather huge.

What is the correct approach to make this cleaner? I could write a unique method for every command, but I already moved all the complicated functionality to their own classes. Does having a new method for a 4-5 lines of code only ever called once really add that much additional cleaneness to the code as a whole?

It seems rather tedious to create unique methods for all these cases, and the switch would still be a bit ugly even if it only allocated two lines to each enum (method call and break). Is there a better way to avoid the switch statement entirely?

How would one try to keep this clean?

ps. this is in java, functional programing is clearly not an option.

marked as duplicate by gnat, user40980, GlenH7, Dan Pichelman, Robert Harvey Feb 7 '14 at 23:07

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

5

My advice would be to leave well alone.

A switch is the most efficient possible branching mechanism especially when coupled with an enum.

Anything else will incur a performance hit.

It may well be worth factoring out the inline code into separate methods for readability ( The JIT compiler will probably just inline them again -- so no overhead here ).

I personally find nothing wrong with two or three line methods if it helps readability.

Also for readability you could consider putting every case on a single line like so:----

switch {humungous_ENUM) {
    case OPTION_1:      exOption_1();            break;
    case OPTION_2:      exOption_2();            break;
     ......
    case OPTION_99:     exOption_99();           break;
    default:            exOption_error();   
}
  • P.S. -- be very, very careful to check for case options without a matching break. This has blown up many a re-factored program. – James Anderson Feb 6 '14 at 10:15
  • performance is actually a non-issue. This code happens once every few seconds at worst, likely less then that. But It sounds like leaving it be due to a lack of a better solution (and effort required to refactor) may be what I have to do. – dsollen Feb 6 '14 at 22:14
4

This is coming from a C# perspective, but what I would do in that case is the following:

  1. Refactor all of the "small bits of code" into separate methods.
  2. Create a dictionary which is keyed on the enum and which contains an Action delegate (a delegate which returns void and is parameterless).
  3. Create a method which adds each enum/delgate pair to the dictionary.
  4. Use the dictionary as a lookup to execute the correct code.

And for more advanced users:

  • Create an attribute which takes the enum value as a parameter. On load, reflect over the assembly and automatically add each enum value/method pair to the dictionary (this will save on all the "adding to dictionary" code).
  • While very clever, the described method is certainly going to result in confusion for many developers that will be following you later. I agree that taken as a stand-alone, this is how I did it one time event, this is understandable. However, when there are hundreds of other kinds of details to be understood by a future developer in a big system, this will certainly make the system much more difficult to understand than a simple switch statement. I don't like the big switch statement but this method is being to clever for your own good. – Dunk Feb 4 '14 at 23:02
  • 1
    That's untrue. The code is more concise, testable and if using the attributing, is actually far easier to read. The attribute tells every developer thereafter how the method is to be used and which enum value will trigger it. I have worked extensively with both systems and the case statements cause far more bugs and are far harder to use than simple attributing. – Stephen Feb 4 '14 at 23:08
  • 1
    I don't doubt more testable. I certainly doubt more understandable. Your method is no different than using arrays of callbacks. I've never seen those used and said to myself, wow, that made things more concise, easier to read or understandable. In fact, quite the opposite. – Dunk Feb 4 '14 at 23:21
  • 1
    Case statements are probably the language feature which leads to the least readable code in my experience. They lead to huge methods that span pages, which is what the OP wanted to avoid. I'm sure that there are other solutions and you're free to post them. – Stephen Feb 4 '14 at 23:42
  • I agree with @Dunk and would add that in most languages, any switch with a more-than-trivial number of cases will end up being handled with a hash. If the OP's code is really only a few lines per case, going through the extra gymnastics to split the code out into a lot of little subroutines and then putting them into a hash will serve to make more clutter. – Blrfl Feb 5 '14 at 0:12
2

You say each case in your big switch corresponds to a public (also published?) API method. From this, I deduce that calls to those API methods are, in reality, calls to some common function with the corresponding enum parameter. I also assume you have the capability to change this public API interface.

It doesn't matter the size of those methods; if they are independent, they should be isolated in separate, public functions. This is true for methods implementing unrelated behaviour, and is orthogonal to their implementation in itself (those classes you mention having all the complicated stuff). A different thing is having several methods with similar behaviour, in which case a single, parameterized method maybe could be added instead. The main guideline in order to reduce the quantity of public functions would be avoiding behaviour duplicity on them.

Last, but not least, try to group semantically related methods (perhaps using static classes), in order to bring in some structure to the otherwise flat array of functions.

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