Refactoring if/else logic

Question

I have a java class with a thousand line method of if/else logic like this:

if (userType == "admin") {
     if (age > 12) {
          if (location == "USA") {
               // do stuff
          } else if (location == "Mexico") {
               // do something slightly different than the US case
          }
     } else if (age < 12 && age > 4) {
          if (location == "USA") {
               // do something slightly different than the age > 12 US case
          } else if (location == "Mexico") {
               // do something slightly different
          }
     }
 } else if (userType == "student") {
     if (age > 12) {
          if (location == "USA") {
               // do stuff
          } else if (location == "Mexico") {
               // do something slightly different than the US case
          }
     } else if (age < 12 && age > 4) {
          if (location == "USA") {
               // do something slightly different than the age > 12 US case
          } else if (location == "Mexico") {
               // do something slightly different
          }
     }

How should I refactor this into something more managable?

Answer 1

You should use Strategies, possibly implemented within an enum, e.g.:

enum UserType {
  ADMIN() {
    public void doStuff() {
      // do stuff the Admin way
    }
  },
  STUDENT {
    public void doStuff() {
      // do stuff the Student way
    }
  };

  public abstract void doStuff();
}

As the code structure within each outermost if branch in your code looks pretty much the same, in the next step of refactoring you might want to factor out that duplication using template methods. Alternatively, you might turn Location (and possibly Age) into a strategy as well.

Update: in Java4, you can implement a typesafe enum by hand, and use plain old subclassing to implement the different strategies.

Answer 2

The first thing I would do with this code is create the types Admin and Student, both of which inherit from the base type User. These classes should have a doStuff() method where you hide the rest of this logic.

As a rule of thumb, any time you catch yourself switching on type, you can use polymorphism instead.

Answer 3

Thousands? Maybe a rules engine is what you need. Drools could be a viable alternative.

Or a Command pattern that encapsulates all the "do something slightly different" logic for each case. Store each Command in a Map with the concatentation of age, location, and other factors as the key. Lookup the Command, execute it, and you're done. Nice and clean.

The Map can be stored as configuration and read in on start up. You can add new logic by adding new classes and reconfiguring.

Answer 4

First - use enums for userType and location - then you can use switch statements (improves readability)

Second - use more methods.

Example:

switch (userType) {
  case Admin: handleAdmin(); break;
  case Student: handleStudent(); break;
}

and later

private void handleAdmin() {
  switch (location) {
    case USA: handleAdminInUSA(); break;
    case Mexico: handleAdminInMexico(); break;
  }
}

Further, identify duplicate code and put it in extra methods.

EDIT

If someone forces you to code Java without enums (like you're forced to use Java 1.4.2), use 'final static's instead of enums or do something like:

  if (isAdmin(userType)) {
    handleAdmin(location, age);
  } else if (isStudent(userType)) {
    handleStudent(location, age));
  }

//...

private void handleAdmin(String location, int age) {
  if (isUSA(location)) {
    handleAdminInUSA(age);
  } else if (isUSA(location)) {
    handleAdminInMexico(age);
  }
}

//...

private void handleAdminInUSA(int age) {
  if (isOldEnough(age)) {
    handleAdminInUSAOldEnough();
  } else if (isChild(age)) {
    handleChildishAdminInUSA(); // ;-)
  } //...
}

Answer 5

The risk of this is not just that it is unsightly, but that it is very error prone. After a while, you could run into a risk of overlaps in your conditions.

If you can really distinguish the condition by user type, you can at the minimum break the body of each condition into a separate function. So that you check based on the type, and call an appropriate function specific to that type. A more OO solution is to represent each user as a class, and then override some calculation method to return a value based on the age. If you can't use classes but can at least use enums, then you will be able to do a nicer switch statement on the enums. Switches on Strings will only come in Java 7.

What worries me is situations of overlaps (e.g., two user types with some shared rules, etc.). If that ends up being the case, you might be better off representing the data as some external file (E.g., a table) which you would read and maintain, and your code will essentially operate as a driver that does the appropriate lookup in this data set. This is a common approach for complex business rules, since nobody wants to go and maintain tons of code.

Answer 6

I would probably first check whether you can parametrize the code doStuff and doSimilarStuff.

Answer 7

You may use Chain of Responsibility pattern.

Refactor if-else statements into classes with an interface IUserController for instance.

Initialize your chain within a list or a tree or any suitable data structure, and execute desired functionality in this chain. You may use Builder pattern to create mentioned data structure. It resembles to strategy pattern but in chain of responsibility pattern, an instance in the chain can call linked instance(s).

Moreover, you can model location specific functionality by using strategy pattern. Hope it helps.

Answer 8

If the code in the blocks fits within a few standard patterns, I would create a table with columns (type, location, minAge, maxAge, action), where 'action' is an enum indicating which of several types of processing to do. Ideally, this table would be read from a data file or kept in SQL.

Then, you can just do a table lookup in the Java code to determine the action to take for a user.

Answer 9

You could make userType an enum, and give it a method that performs all of your "do something slightly different" actions.

Answer 10

without more information there is no good answer

but fair guess would be this: use OO

first define a User, define Admin, Student and all other types of users and then let polymorphism take care of the rest

Answer 11

Based just on the variable names, I'm guessing that you should subclass User (or whatever it is that has a userType variable) into AdminUser and StudentUser (and possibly others) and use polymorphism.

Answer 12

Take a look at the Visitor pattern. It makes use of polymorphism but is a little more flexible in that it is easier to add new cases later.

The downside is you'd need some way to convert the state info into different instances. The benefit is a cleaner way to add behavior without having to modify your inheritance hierarchy.

Answer 13

You really need to break these cases into object methods. I'm assuming these strings and numbers are being pulled out of a database. Instead of using them in their raw form in giant nested conditional logic, you need to use these pieces of data to construct objects that model the desired interactions. Consider a UserRole class with a StudentRole and AdminRole subclasses, a Region class with USA and Mexico subclasses, and an AgeGroup class with appropriate partitioned subclasses.

Once you have this object oriented structure in place, you'll be able to make use of well understood object oriented design patterns to re-factor this logic.

Answer 14

Use OOP Concepts: This is dependent of the rest of the design, but maybe you should have a user interface, Student,Admin interfaces the extends it and UsaStudent,MexicoStudent,UsaAdmin,MexicoAdmin implementation that do some stuff. Hold a User instance and just call its doStuff method.