Java Method Refactoring Using Enum

Question

The getCategory method below seems very redundant and I was wondering if anyone has some suggestions on refactoring it to make it cleaner possibly using an Enum. Based on the "val" passed in, I need getCategory to return the proper Category instance from the Category class. The Category class is generated JNI code, so I don't want to change that. Anyone have any ideas?

Method to be refactored:

private Category getCategory(String val) throws Exception{
        Category category;
        if (val.equalsIgnoreCase("producer")) {
            usageCategory = Category.CATEGORY_PRODUCER;
        } else if (val.equalsIgnoreCase("meter")) {
            usageCategory = Category.CATEGORY_METER;
        } else if (val.equalsIgnoreCase("consumer")) {
            usageCategory = Category.CATEGORY_CONSUMER;
        } else {
            throw new Exception("Invalid value: " + val);
        }       
        return usageCategory;
    }

Category.java: Generated JNI (can't change this):

public final class Category {
  public final static Category CATEGORY_PRODUCER = new Category("CATEGORY_PRODUCER", SampleJNI.CATEGORY_PRODUCER_get());
  public final static Category CATEGORY_METER = new Category("CATEGORY_METER", SampleJNI.CATEGORY_METER_get());
  public final static Category CATEGORY_CONSUMER = new Category("CATEGORY_CONSUMER", SampleJNI.CATEGORY_CONSUMER_get());

}

Answer 1

Your method is essentially mapping from a predetermined String to a Category, so why not use a Map instead? Specifically, I'd recommend Guava's ImmutableMap, since these mappings are static:

private static final ImmutableMap<String, Category> CATEGORIES_BY_STRING =
        ImmutableMap.of(
            "producer", Category.CATEGORY_PRODUCER,
            "meter", Category. CATEGORY_METER,
            "consumer", Category.CATEGORY_CONSUMER
        );

Or the standard way if you don't want to use a third-party library:

private static final Map<String, Category> CATEGORIES_BY_STRING;
static {
    Map<String, Category> backingMap = new HashMap<String, Category>();
    backingMap.put("producer", Category.CATEGORY_PRODUCER);
    backingMap.put("meter", Category.CATEGORY_METER);
    backingMap.put("producer", Category.CATEGORY_CONSUMER);
    CATEGORIES_BY_STRING = Collections.unmodifiableMap(backingMap);
}

You could still employ your method to check for invalid values (and support case-insensitivity as David Harkness pointed out):

private Category getCategory(String val) {
    Category category = CATEGORIES_BY_STRING.get(val.toLowerCase());
    if (category == null) {
        throw new IllegalArgumentException();
    }
    return category;
}

---

About using enums:

If you have complete control over the Strings that are passed into getCategory, and would only be passing literal values, then it does make sense to switch to an enum instead.

EDIT: Previously, I recommended using an EnumMap for this case, but Adrian's answer makes much more sense.

Answer 2

If you said you want to refactor base on enum, I assume you mean you no longer want to pass in the String to getCategory to do all those work. The code is using enum directly, instead of using String.

If this is the case, continue reading

Luckily, your Category is static variables, so you can do something real straight-forward

public enum FooCategory {  // give a better name yourself
  PRODUCER(Category.CATEGORY_PRODUCER),
  METER(Category.CATEGORY_METER),
  CONSUMER(Category.CATEGORY_CONSUMER)

  private Category category;
  FooCategory(Category category) {
    this.category=category;
  }
  Category getCategory() {
    return this.category;
  }
}

In your old code, you are doing something like:

String fooCategory = "producer";
//....
Category category = getCategory(fooCategory);
// work on category

Now you are doing something much neater

FooCategory fooCategory = FooCategory.PRODUCER;
//...
Category category = fooCategory.getCategory();
// work on category

Answer 3

@PaulBellora and @AdrianShum 's answers are both great, but I think you can't avoid using magic value just like 'producer'(case-insensitive) to produce Category for storage reason. So I'm afraid the redundant code in getCategory can't avoid either. (Unfortunately, the magic values used to init Category and get Category are not same)

Here are the code to use Enum(Java 1.5 or above):

public enum Category {
    CATEGORY_PRODUCER,
    CATEGORY_METER,
    CATEGORY_CONSUMER;

    public static Category of(String val) throws Exception {
        Category usageCategory;
        if (val.equalsIgnoreCase("producer")) {
            usageCategory = Category.CATEGORY_PRODUCER;
        } else if (val.equalsIgnoreCase("meter")) {
            usageCategory = Category.CATEGORY_METER;
        } else if (val.equalsIgnoreCase("consumer")) {
            usageCategory = Category.CATEGORY_CONSUMER;
        } else {
            throw new Exception("Invalid value: " + val);
        }       
        return usageCategory;
    }
}

If you use the same magic value to produce, fetch and store, you can:

public enum Category {
    CATEGORY_PRODUCER("producer"),
    CATEGORY_METER("meter"),
    CATEGORY_CONSUMER("consumer");

    private String category;
    private Category(String category) {
        this.category = category;
    }

    public static Category of(String val) throws Exception {
        Category usageCategory;
        // You can use equals not equalsIgnoreCase, so you can use map to avoid redundant code.
        // Because you use the same magic value everywhere.
        if (val.equals("producer")) {
            usageCategory = Category.CATEGORY_PRODUCER;
        } else if (val.equals("meter")) {
            usageCategory = Category.CATEGORY_METER;
        } else if (val.equals("consumer")) {
            usageCategory = Category.CATEGORY_CONSUMER;
        } else {
            throw new Exception("Invalid value: " + val);
        }       
        return usageCategory;
    }
}

and create a Category as:

Category category = Category.of("producer");

Of course, you have to change the code to include SampleJNI.CATEGORY_CONSUMER_get().