CODEWITHSUNDEEP
java
pathfinder
pathfinder

Reputation: 127

Enum vs If-else

I have a requirement wherein I need to build an employee object as below from an event list. Currently I've written my code as below, but QE gave a comment saying possible use of enums instead of multiple if else's. Can someone suggest me on how to achieve this with enums.

Employee e= new Employee();
for(Event event:events){
if("empid".equals(event.getName())
   e.setEmployeeId(event.getvalue());
else if("empname".equals(event.getName())
   e.setEmployeeName(event.getvalue());
else if("empsal".equals(event.getName())
   e.setEmployeeSal(event.getvalue());
else if("empdob".equals(event.getName())
   e.setEmployeeDOB(event.getvalue());
else if("emprole".equals(event.getName())
   e.setEmployeeRole(event.getvalue());
}

Upvotes: 3

Views: 3029

Answers (3)

Adrian Shum
Adrian Shum

Reputation: 40036

If you are in control of development of Event, I believe what your QE saying is to replace event name by an enum (which is a sane design as you have already decide what is the possible types of event). However, if Event's design is out of your control, or you cannot have a child class of Event for your use (e.g. make an EmployeeEvent), then just ignore what I am going to say)

i.e. 

enum EventType {
    EMP_ID,
    EMP_NAME,
    ....
}


interface Event {
    EventType getType();   // instead of getName() which returns a String
}

Then your code can be simplified to

Employee e= new Employee();
for (Event event: events) {
    switch (event.getType()) {
        case EMP_ID:
            e.setEmployeeId(event.getvalue());
            break;
        case EMP_NAME:
            e.setEmployeeName(event.getvalue());
            break;
        ....
    }
}

You may even preset the action to do against each event type, using a map (which is of similar idea of another answer)

Map<EventType, BiConsumer<Employee, String>> eventActions = new EnumMap<>();
eventActions.put(EventType.EMPLOYEE_ID, Employee::setEmployeeID);
eventActions.put(EventType.EMPLOYEE_NAME, Employee::setEmployeeName);

so you can further simplify the above switch by:

Employee e= new Employee();
for (Event event: events) {
    eventActions.get(event.getType()).accept(e, event.getValue()));
}

Upvotes: 3

sprinter
sprinter

Reputation: 27946

I would suggest you move the logic of what to do with events into the enum. If you are using Java 8 then it would look something like:

enum EmployeeField {
    ID("empid", Employee::setEmployeeID),
    NAME("empname", Employee::setEmployeeName),
    SALARY("empsalary", Employee::setEmployeeSalary),
    ...

    private final String key;
    private final BiConsumer<Employee, String> valueSetter;

    EmployeeField(String key, BiConsumer<Employee, String> valueSetter) {
        this.key = key;
        this.valueSetter = valueSetter;
    }

    public void setEmployeeField(Employee employee, String value) {
        valueSetter.accept(employee, value);
    }

    public static EmployeeField getFieldForKey(String key) {
        return Arrays.stream(values[])
            .filter(ef -> ef.key.equals(key))
            .findAny()
            .orElseThrow(new IllegalArgumentException("No employee field " + key));
    }
}

Then you can dispense with the switch statement altogether and just use:

events.stream()
    .forEach(ev -> EmployeeField.getFieldForKey(ev.getName())
        .setEmployeeField(emp, ev.getValue()));

This also means that all the information about employee fields, including how to set employee values, is encapsulated in the enum and can be easily changed or extended without anything else being impacted.

Note that you can do a similar thing prior to Java 8 without using the lambdas but it's not as elegant (in my view) as the anonymous interface instances need to become explicit which makes the code a lot more complicated. Or you can override methods for each enum member which (in my view) is even uglier.

Upvotes: 2

Vivek Singh
Vivek Singh

Reputation: 2073

Create an enum with the below codes 

public enum  EMPLOYEE_FIELDS {EMPID, EMPNAME,EMPSAL, EMPDOB, EMPROLE};

EMPLOYEE_FIELDS empField = EMPLOYEE_FIELDS.valueOf(event.getName().toUpperCase());

 switch(empField ){
    case EMPID:
        //do something here
        break;
    case EMPNAME:
        //do something here
        break;
    // other cases.
    // other cases.
    // other cases.
    default:
            //do something here
 }

Hope this helps!

Upvotes: 2

Related Questions