Can this code be written in a better and more flexible way?

Question

Please see the code below . Here based on the String constants , I am instantiating different types of component classes . Now there are atleast 15 different types of String constants . So if I follow this pattern there will be 15 different cases and those many if -else blocks . Is there a better way of doing this ? I want to have the flexibility of being able to add and delete cases by doing the minimum possible code change.

public UIComponent initCellEditor(String editorType) {
        UIComponent editControl = null;
        if ("TbComboBoxCellType".equals(editorType)) {
          editControl = new WebListEntryField();
          editControl.setId("ComboBox");
        } else if ("TbStringCellType".equals(editorType)) {
          editControl = new WebInputEntryField();
          editControl.setId("String");
        } else if ("TbDateCellType".equals(editorType)) {
          editControl = new WebDateEntryField();
          editControl.setId("Date");
        } else if ("TbDateTimeCellType".equals(editorType)) {
          editControl = new WebDateTimeEntryField();
          editControl.setId("DateTime");
        } else {
          //default editor is allways a text input
          editControl = new WebInputEntryField();
          editControl.setId("Input");
        }
        return editControl;
      }

P.S: We are using JDK 6 . So can't use switch on String feature.

use a `switch`? http://docs.oracle.com/javase/tutorial/java/nutsandbolts/switch.html — Ron van der Heijden, Sep 18 '12 at 09:48
Switch on string is only possible in Java 7 and later. If it has to be Strings here, you could switch on their hash? — Fildor, Sep 18 '12 at 09:49

score 8 · Accepted Answer · answered Sep 18 '12 at 09:54

8

you could transform those string constants to Enums, and add a builder method on the enum, for example

public enum CellType {
      TbComboBox {
         @Override
         public UIComponent newComponent() {
            return new xxx();
         }
      },
      TbDate {
         @Override
         public UIComponent newComponent() {
            return new xxx();
         }
      }

      public abstract UIComponent newComponent();
}

The beauty of this approach is that it replaces IFs with polymorphism (which in my opnion is very OO).

Sorry, I just realised that you have a default type (the last else in your code), so you might need to add an if somewhere :(.

answered Sep 18 '12 at 09:54

Augusto

28,839
5
58
88

I still have to have the call to initCellEditor . So what will the method initCellEditor look like with this approach ? Also can I have this enum as a static nested class inside another public class? – Geek Sep 18 '12 at 10:09
You would need to convert `editorType` from string to an Enum everywhere. It's usually a bad practice to represent types as strings. If you do this, then `initCellEditor` disappears completely. Please bear in mind that this solution might not be right for you, I've done things like this in the past and it worked for me. – Augusto Sep 18 '12 at 10:14
Geek! I forgot to say, that this solution also abides by the [Open/Closed Principle](http://en.wikipedia.org/wiki/Open/closed_principle)! :) – Augusto Sep 24 '12 at 19:49

score 4 · Answer 2 · answered Sep 18 '12 at 09:53

Maybe: Use a Map. This approach has the possible advantage of working even if you create new classes, as long as you inject the new type into the Map. It's got a dependency injection flavor to it.

package cruft;

import java.util.HashMap;
import java.util.Map;

/**
 * UIComponentFactory description here
 * @author Michael
 * @link
 * @since 9/18/12 5:48 AM
 */
public class UIComponentFactory {
    Map<String, UIComponent> uiComponentMap;
    Map<String, String> uiIdMap;

    public UIComponentFactory(Map<String, UIComponent> uiComponentMap, Map<String, String> uiIdMap) {
        this.uiComponentMap = new HashMap<String, UIComponent>(uiComponentMap);
        this.uiIdMap = new HashMap<String, UIComponent>(uiIdMap);
    }

    public UIComponent initCellEditor(String editorType) {
        UIComponent editControl = null;
        editControl = this.uiComponentMap.get(editorType);
        if (editControl != null) {
            editControl.setId(this.uiIdMap.get(editorType));
        } else {
            editControl = new WebInputEntryField();
            editControl.setId("Input");
        }
        return editControl;
    }
}

Look again, Matt. I pass in editorType and I use it as the key to the Map when I look up the returned value. Did you even read the code? — duffymo, Sep 18 '12 at 12:19

assylias · Answer 3 · 2012-09-18T10:14:43.153

You could use an enum:

public static enum Editor {

    TB_COMBOBOX_CELL("tbComboBoxCellType", "ComboBox") {
        public UIComponent getComponent() {
            return new WebListEntryField();
        }
    },
    TB_STRING_CELL("TbStringCellType", "String") {
        //etc
    };
    private final String type;
    private final String id;

    private Editor(String type, String id) {
        this.type = type;
        this.id = id;
    }

    public String getType() {
        return type;
    }

    public String getId() {
        return id;
    }

    public abstract UIComponent getComponent();
    private static Map<String, Editor> types = new HashMap<String, Editor>();

    static {
        for (Editor e : Editor.values()) {
            types.put(e.getType(), e);
        }
    }

    public static Editor getEditor(String type) {
        Editor e = types.get(type);
        if (e == null) return Editor.DEFAULT_EDITOR;
        return e;
    }
}

Then your method becomes:

public UIComponent initCellEditor(String editorType) {
    Editor e = Editor.getEditor(editorType);
    UIComponent editControl = e.getComponent();
    editControl.setId(e.getId());
    return editControl;
}

@Geek just add a `DEFAULT_EDITOR` to your enum list, with the proper `getComponent` method... — assylias, Sep 18 '12 at 13:02

score 1 · Answer 4 · answered Sep 18 '12 at 09:57

You can use a Factory pattern to create objects depending on their types. For e.g.:

Create a factory class something like this:

public class UIComponentFactory {

    private static final Logger LOGGER = LoggerFactory.getLogger(UIComponentFactory.class);

    private static UIComponentFactory instance;

    private static final LinkedHashMap<String, Class> COMPONENTS_MAP = new LinkedHashMap<String, Class>() {{
        put("TbComboBoxCellType", WebListEntryField.class);
        put("TbStringCellType", WebInputEntryField.class);
        put("TbDateCellType", WebDateEntryField.class);
    }};

    private UIComponentFactory() {
    }

    public UIComponent createUIComponent(String type) {
        Class componentClass = COMPONENTS_MAP.get(type);
        Object componentObject = null;
        if (componentClass != null) {
            try {
                componentObject = componentClass.newInstance();
            } catch (InstantiationException ex) {
                LOGGER.error("Instantiation exception occurred", ex);
                throw new SystemException("Instantiation exception occurred", ex);
            } catch (IllegalAccessException ex) {
                LOGGER.error("Illegal access exception occurred", ex);
                throw new SystemException("Illegal access exception occurred", ex);
            }
        }
        return (UIComponent) componentObject;
    }

    public static UIComponentFactory getInstance() {
        if (instance == null) {
            instance = new UIComponentFactory();
        }
        return instance;
    }

}

Then to create your component use:

UIComponentFactory factory = UIComponentFactory.getInstance();
UIComponent component = factory.createUIComponent("yourComponentType");

This would be a more efficient way then using if else statements.

score 1 · Answer 5 · answered Sep 18 '12 at 10:00

1

To be honest: Don't change anything. There are a couple of options to refactor this code, but IMHO they won't improve the readability of your code. Stick with if, it's not as evil as they want you to believe...

answered Sep 18 '12 at 10:00

Matt

17,290
7
57
71

score 1 · Answer 6 · answered Sep 18 '12 at 10:01

I would suggest an enum based approach as follows:

enum EditorType {
    TbComboBoxCellType(WebListEntryField.class, "ComboBox"),
    TbStringCellType(WebInputEntryField.class, "String"),
    TbDateCellType(WebDateEntryField.class, "Date"),
    TbDateTimeCellType(WebDateTimeEntryField.class, "DateTime"),
    Generic(WebInputEntryField.class, "Input");

    private final Class<? extends UIComponent> componentType;
    private final String id;

    private EditorType(Class<? extends UIComponent> componentType, String id) {
        this.componentType = componentType;
        this.id = id;
    }

    public static UIComponent createComponent(String editorType) {
        EditorType type;
        try {
            type = valueOf(editorType)
        } catch (IllegalArgumentException e) {
            type = Generic;
        }
        return type.createComponent();
    }

    public UIComponent createComponent() {
        try {
            UIComponent component = componentType.newInstance();
            component.setId(id);
            return component;
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }

}

score 0 · Answer 7 · answered Sep 18 '12 at 09:51

0

You should not create new WebListEntryField(); or other xxEntryField() each time. Create a private field and return the same instance.

answered Sep 18 '12 at 09:51

basiljames

4,777
4
24
41

2

That's not an answer to the question. – Fildor Sep 18 '12 at 09:52

score 0 · Answer 8 · answered Sep 18 '12 at 09:52

0

Another option is to use a Enum to capture all

CellTypes

and another or the same enum to get the Ids and use that Enum to compare the input coming in.

answered Sep 18 '12 at 09:52

Binu

120
6

score 0 · Answer 9 · edited May 23 '17 at 12:21

0

Use switch on enums. Change String editorType to emum type. If this is impossible, convert editorType to enum as in following SO question.

edited May 23 '17 at 12:21

Community

1
1

answered Sep 18 '12 at 09:55

Alexei Kaigorodov

13,189
1
21
38

score 0 · Answer 10 · answered Sep 18 '12 at 11:13

you can use enum based comparison. Create different enum for different input type and then use switch case.

EditorType.java

public enum EditorType {
  COMBO_CELL_TYPE("TbComboBoxCellType"),
  STRING_CELL_TYPE("TbStringCellType"),
  DATE_CELL_TYPE("TbDateCellType"),
  TIME_CELL_TYPE("TbDateCellType"),
  INPUT_CELL_TYPE("TbInputCellType");

  public String value;

  private EditorType(String value) {
    this.value = value;
  }

}

While compare use switch statement.

public UIComponent initCellEditor(EditorType editorType) {
  UIComponent editControl = null;

  switch (editorType) {
    case COMBO_CELL_TYPE:
      // logic here
      break;

    case STRING_CELL_TYPE:

      break;

    case DATE_CELL_TYPE:

      break;

    case TIME_CELL_TYPE:

      break;

    default:
      // default logic
  }
}

Can this code be written in a better and more flexible way?

10 Answers10