Refactoring a long method which is based on large number of switch cases

Question

We are using Java as a backend development language.

One year back, we wrote a method which uses switch cases based on Enums values. Since we are continuously adding enum members and according adding cases in the method, the method has grown to very large extent. Currently, we have around 100 enum fields and corresponding number of switch cases.e.g.

 class AClass{

   enum option{ o1, o2, o3...on}

   Value method(Option o){

      switch(o){
        case o1:
          value = deriveValue(p1,p2,p3);
        case o2:
          value = deriveValue(p2,p3,p4);
        .
        .
        .
        case on:
          value = deriveValue(p1,p2,p3);
      }
   } 
 }

Thus each time a business requirement comes, we add enum and corresponding switch case. Now the method has become too long and looks unmanageable, if we keep on adding the same logic in future.

To clean, we thought of replacing switch case with polymorphism by creating classes for the same, but again, we will end up creating n number of classes.

We are looking for small, simple and manageable solution.

----------------- Update ---------------------

As suggested, to elaborate more, Enum values are fields names for which a client needs a value. Thus if a client need the value of a new field, we add the field in enum, and the corresponding definition of how to fetch value for the newly added field, by adding the corresponding switch case.

In the switch case, we have a common reusable method e.g. deriveValue() (Please refer to the example given) to which we pass parameters required for deriving the value for the newly added field.

Answer 1

What are you trying to achieve? Are you having difficulty understanding and maintaining the code structure you have, or are you just trying to follow some abstract idea of "good practice" that says methods should be short?

It looks to me as if the solution you have is quite adequate for the requirements you've been given. If there really are hundreds of similar computations that can be performed by a single line of code, then creating hundreds of subclasses with a single small method each is not necessarily better than simply listing all those parallel rules in one place. (The trouble with long methods is that they will become repetitive, inconsistent, and therefore confusing. But for listing a lot of parallel cases that have to be handled, a long list is perfectly fine.)

Answer 2

You may create a table. The first column will be the switch value and the other columns will be the arguments passed on the deriveValue method. For example

table OPTIONS
OPTION -  ARGUMENT1 -  ARGUMENT2 -  ARGUMENT3
o1          p1          p2          p3
o2          p2          p3          p4
on          p1          p2          p3

You can then write one general function that uses the above table to produce the appropriate function call based on the option.

Answer 3

You could use abstract methods in the enum, thereby having better code locality (for better legibility and maintenance, not talking performance here).

class AClass{
    enum Option{
        o1 {
            @Override
            Value deriveValue() {
                return deriveValue(p1, p2, p3);
            }
        }, ... on {
            @Override
            Value deriveValue() {
                return deriveValue(p1, p2, p3);
            }
        };
        abstract Value deriveValue();
    }

    Value method(Option o){
        return o.deriveValue();
    }
}

Answer 4

It's worth noting that, if you have this kind of situation where there's only one single function in the system always which will perform such checks, and it does not have any need whatsoever to be open for external extension, then throwing a polymorphic solution at it may actually leave you finding that your daily maintenance costs have actually increased rather than decreased.

Here, excluding a need for open extension, the practical difference between polymorphic and conditionals might boil down to whether you prefer more centralized, unstable code or more decentralized, stable code.

More decentralized, stable code might actually be favorable in some cases if it leads to less version control toe-stepping, e.g. More centralized, unstable code here would tend to be shorter, if only as a result of avoiding the boilerplate associated with defining new classes for each little simple case.

Currently, we have around 100 enum fields and corresponding number of switch cases.e.g. Thus each time a business requirement comes, we add enum and corresponding switch case.

This feels like the problem to me if we can avoid it. Is there no kind of categorization that can accompany so many enum fields? With the kind of ordinal example you gave, that wouldn't find much meaningful categorization. I'm hoping it was just an analogical one.

If you can find some kind of meaningful categorization to organize these enum fields, you can potentially use more than one function. Each function (still pretty coarse) could handle a range of cases, with an outer function checking which function to call based on the range.

That's a simple and not-so-intrusive solution, provided you can find some sort of categorization, to still have a fairly centralized and simple solution, but leave behind stable functions for the range of options you've already handled. It'll also reduce the instability of the function implementations involved, since the fewer range of options they handle, the fewer reasons they'll find to individually change.

You can also potentially use look-up table solutions possibly without a subtype involved for all 100 options, which gives you one centralized place to modify but possibly with a possibly more-preferable syntax. It depends on the nature of p1, p2, ..., pn. For example, if they are homogeneous, you might be able to use an array and make a LUT based on the index ordering with option enum fields modeling an index into the LUT. Example (pseudocode):

lut[o1] = {0, 1, 2};
lut[o2] = {1, 2, 3};
...
lut[on] = {0, 1, 2};

Value method(Option o){
    int i1 = lut[o][0];
    int i2 = lut[o][1];
    int i3 = lut[o][2];
    deriveValue(p[i1], p[i2], p[i3]);
}

This doesn't really do much architecturally (though it is open for extension if you allow lut entries to be added from the outside) but you might prefer the syntax. It applies if your solution wasn't a remote analogy and actually just calls deriveValue based on the option with a different ordering of arguments (in which case I'd think there would be at least more than 3 parameters in actuality to call for so many options).

Answer 5

You could apply the general strategy used to deal with big numbers -- structure. Perhaps the prolific number of options handled uniformly is a sign of too little hierarchy? Perhaps the cases fall neatly in groups which seem to belong together (like I/O, manipulation, computation etc.)? (Perhap not -- then please ignore ;-) ).

This could be reflected with a a hierarchical enum system. One enum characterizes the group, the next one the specific operation.

The groups could be coded separately and would each be more managable. Having them in different source files/classes also makes editing errors in unrelated places less likely, may speed up compilation and should reduce dependencies (from system libraries as well as from other custom classes).

Of course this approach is also conceivable as a design idea for polymorhpism: One could have specific option/action classes deriving from group classes deriving from a generic option class), with an opportunity to bundle common code further up in the hierarchy.

Answer 6

Changing to classes won't help you. In terms of clarity, robustness, compactness, maintainability and performance it doesn't get any better than enums and a switch statement. What you should be asking yourself is if you are dealing with types at all. Considering that you are frequently and continuously adding enums, it seems to me you are dealing with data, not types. You might need a single class and multiple instances. This is what DesignerAnalyst implies with the table proposal. You may still hard-code your input data for your object instances or slap the input in a text file, the big question remains: what ARE those enum values really? How do they classify?

Answer 7

Adding more and more classes is not really a problem, if they are parts of a well defined structure that allows you to ignore them when you want to: just put them in a module, and only access the module when you have to modify it.

IMHO, each option represents a way to compute a value, therefore in OO, each opinion should be represented by an object having a specific implementation of deriveValue(). In Java, that means that each object must have a specific class defining the implementation. So your code becomes something like:

In your option package:

public interface Option {
     Value deriveValue(? p1, ? p2, ? p3);
}

public FirstOption implements Option {
    Value deriveValue(? p1, ? p2, ? p3){ ... } 
}

In an other package;

class AClass{
   private Option o;

   Value method(){ o.deriveValue(p1,p2,p3); }
}

If you have different behaviors to specify, each of them can be represented by a single call instead of a big switch. Caveat: each time you want to add a new behavior, you have to open each option class to add a new method.

Answer 8

You have more and more options, leading to more and more enum cases, and more and more code implementing something for each option. You can try to do something about the growing number of options - well, maybe you can't.

If you can't prevent the number of options from growing, then a switch statement is just as good as any other method to choose between the different options. And it looks like very little additional code is needed beyond the actual code to evaluate each option. A decent compiler will tell you about missing cases.

If you don't like this advice: One day you reach the point where you worry about simple and maintainable code, and not about violations of some abstract principle that you just heard about at your latest course. Rules are there to be broken unless you are a novice. Now if instead of "this is a violation of the open-closed principle" you could actually give a practical example...