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.

gnat
  • 21,442
  • 29
  • 112
  • 288
dsollen
  • 1,123
  • 1
  • 12
  • 28
  • add a code sample – TruthOf42 Feb 04 '14 at 21:53
  • Why is the switch statement ugly? – Michael Shaw Feb 04 '14 at 22:19
  • 1
    possible duplicate of [Refactoring Switch Statements and is there any real use for Switch Statements at all?](http://programmers.stackexchange.com/questions/147214/refactoring-switch-statements-and-is-there-any-real-use-for-switch-statements-at) and of [Why use an OO approach instead of a giant “switch” statement?](http://programmers.stackexchange.com/questions/79111/why-use-an-oo-approach-instead-of-a-giant-switch-statement) – gnat Feb 06 '14 at 05:35
  • I recommend reading a chapter from Martin Fowler's book "Refactoring: Improving the Design of Existing Code" (Chapter 9. Simplifying Conditional Expressions). There, he presents several options, depending on the requirements, size of problem, time you are willing to invest, etc. – braikin Feb 06 '14 at 05:08
  • Do these methods rely on a lot of local lexical state, or could you generalize them to a common interface? – BRPocock Feb 06 '14 at 06:04
  • Is this Swing? If so, use Actions. – user949300 Feb 06 '14 at 07:11

3 Answers3

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();   
}
James Anderson
  • 18,049
  • 1
  • 42
  • 72
  • 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 06 '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 06 '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).
Stephen
  • 8,800
  • 3
  • 30
  • 43
  • 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 04 '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 04 '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 04 '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 04 '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 05 '14 at 00:12
  • +1 as the advanced C# user I agree 100% on both approaches being the best available simple solutions for those capable of implementing them, and anyone should be capable of the first - even in Java using a strategy pattern with anonymous classes this should be simple and clean. – Jimmy Hoffa Feb 05 '14 at 20:07
  • Unforunately I think the argument as to rather or not this is a better option may be a moot one. Java doesn't really alow functional programing, at least not cleanly. While theoretically possible doing this in java gets quite verbose, ugly and confusing due to limitations in Java (at least until 1.8) – dsollen Feb 06 '14 at 22:11
  • Without the ability to use delegates, I would use a command pattern to create classes for all of the methods with the code inside an Execute() method. You could then store all of these command classes inside your dictionary and call the Execute() method of each command. – Stephen Feb 07 '14 at 00:31
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.

rucamzu
  • 525
  • 2
  • 10