5
\$\begingroup\$
class MyObject
{
    public static enum Type {A, B, C, D;}

    public static final int ID_MAIN = 1;
    public static final int ID_MAIN_UK = 2;
    public static final int ID_MAIN_US = 3;
    public static final int ID_SUB = 4;
    // lots more constants here

    public static final String DESCRIPTION_1 = "Desc Full Name";
    public static final String DESCRIPTION_2 = "Desc2 Full Name";
    // lots more constants here

    private int id;

    public MyObject(final int id)
    {
        this.id = id;
    }

    //simple getter 
    public int getID() { return this.id;}

    // real responsibility of the class is in the following two methods
    public static String getDescription()
    {
         switch(id)
         {
              case MyObject.ID_MAIN:
              case MyObject.ID_MAIN_UK:
                  return MyObject.DESCRIPTION_1;
              case MyObject.ID_SUB:
                  return MyObject_Description_2;
              default:
                   // throw IllegalArgException
         }        
    }

    public static Type getType(int id)
    {
         switch(id)
         {
             case MyObject.ID_MAIN:
             case MyObject.ID_SUB:
                 return Type.A;
             case MyObject.ID_MAIN_UK:
             case MyObject.ID_MAIN_US:
                 return Type.B;
             default:
                 return Type.Undefined;
         }
     }
 }

Basically, there is an ID that maps to both a description and a type. This ID is passed in during construction of the class and it should map to a set of constants already contained in the class. If the id is not part of the list of constants, an error is thrown when trying to get the description that maps to the id and an 'Unknown' type is return if the type is queried. The ID maps a description to a set of constants. The same ID maps to a certain Type (defined as an enum).

This code is pretty ugly because there are tons of constants defined at the top, which makes the switch statements pretty bloated. Is there a simple way to refactor this without changing the public interface? It seems trivially simple, but it seems pretty ugly no matter how you slice it. How can I simplify these mappings to make the code more concise?

I was thinking about representing the mappings in a text file and having a manager class that held simple containers in a hashmap. When the manager class is constructed, it would create the objects by reading the text file and map them to an ID. When the manager is queried with the ID, it would just call the corresponding get method, for instance:

class Manager 
{
     private HashMap<int, MyObject> objectMap;

     public Manager() {} //construct the object map
     public String getDescription(int id) { return objectMap.get(id).getDescription();}
     public Type getType(int id) { return objectMap.get(id).getType();}
}

class DataContainer
{
     private String description;
     private Type type;


     public DataContainer(String desc, Type type) {//set mem vars}
     public String getDescription() //simple getter
     public Type getType() //simple getter
 }

But this solution seems too complicated. Is there a better solution, preferably one that would keep everything in one class?

\$\endgroup\$
1
  • \$\begingroup\$ Static methods cannot possibly switch on instance variable id. \$\endgroup\$ Jan 24, 2014 at 6:04

1 Answer 1

2
\$\begingroup\$

This is a classic case for using an Enum....

public enum MyEnum {
    ID_MAIN("Desc Full Name", Type.A),
    ID_MAIN_UK("Desc Full Name", Type.B),
    ID_SUB("Desc1 Full Name", Type.A),
    .....



    private final String description;
    private final Type type;

    private MyEnum(String desc, Type type) {
        this.type = type;
        this.description = desc;
    }

    public int getID() {
        return ordinal() + 1;
    }

    public String getDescription() {
        return description;
    }

    public Type getType() {
        return type;
    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ The sample code a) already uses an enum and b) didn't look so simple. Each enum didn't map directly to an ID one greater. A map or two are required here, encapsulated within an ID-to-enum-and-description helper. \$\endgroup\$ Jan 24, 2014 at 6:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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