HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Picking a pattern based on employee type or department

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
employeepickingtypebaseddepartmentpattern

Problem

I have an if else statement like this deciding the pattern to be followed based on type or department of employee.

The if else block goes like this. I am planning to simplify the if else block to a better format.

public static Pattern decidePattern(Employee employee) {
        try {
            if ("individual".equalsIgnoreCase(employee.getType()) {
                return new IndivualPattern();
            } else if ("multiple".equalsIgnoreCase(employee.getType()) {
                return new MultiplePattern();
            } else if ("combined".equalsIgnoreCase(employee.getType()) {
                return new CombinedPattern();
            }else if ("financial".equalsIgnoreCase(employee.getDepartment()) {
                return new FinancialPattern();
            } else if ("hr".equalsIgnoreCase(employee.getDepartment()) {
                return new HrPattern();
            } else if ("facilities".equalsIgnoreCase(employee.getDepartment()) {
                return new FacilitiesPattern();
            } else {
                return new DefaultPattern();
            }
        } catch (Exception e) {
            return new DefaultPattern();
        }
    }


All patterns are inherited from common interface. Can I break this into a simpler one?

Solution

You can't really do much to simplify this. There are 6 cases plus one default which means 7 different results, which means 7 lines to return and 6 conditions.

Unless you don't need to return a new thing every time. Assuming Pattern is from java.util.regex, this is the case. Then you can use something like

import com.google.common.collect.ImmutableMap;

private static final Map PATTERNS_FOR_TYPE =
    ImmutableMap.of(
    "individual", new IndivualPattern(),
    "multiple", new MultiplePattern(),
    "combined", new CombinedPattern());
private static final Map PATTERNS_FOR_DEPARTMENT = ...;

public static Pattern decidePattern(Employee employee) {
    Pattern result = PATTERNS_FOR_TYPE
            .get(employee.getType().toLowerCase(Locale.ENGLISH));
    if (result!=null) {
        return result;
    }
    Pattern result = PATTERNS_FOR_DEPARTMENT
        .get(employee.getDepartment().toLowerCase(Locale.ENGLISH));
    if (result!=null) {
        return result;
    }
    return new DefaultPattern();
}


But I wouldn't call it a big improvement. What you should do instead or in addition to it, is to get rid of the Strings, that's what enums are for.

In case you insist on Strings, you should normalize them ASAP. No equalsIgnoreCase, no toLowerCase anywhere but when when you encounter the inputs the first time.

Concerning catch Exception, no method can really throw an all possible Exceptions and you should be as specific as possible.

You can (and most probably should) use enums. The annotations come from project Lombok and do exactly what they say (you can do it manually, too).

@RequiredArgsConstructor @Getter 
enum Type {
    INDIVIDUAL(new IndivualPattern()),
    MULTIPLE(new MultiplePattern()),
    COMBINED(new CombinedPattern()),
    ANOTHER(null),
    YET_ANOTHER(null);

    private final Pattern pattern;
}

... enum Department ...

public static Pattern decidePattern(Employee employee) {
    Pattern result = employee.getType().getPattern();
    if (result!=null) {
        return result;
    }
    ...
}


Not always it is a good idea to define the pattern in the enum itself. Then you can use a switch

public static Pattern decidePattern(Employee employee) {
    switch (employee.getType()) {
        case INDIVIDUAL: return new new IndivualPattern();
        ...
        default: // (just to prevent "missing label" warning)
    }
    switch (employee.getType()) {
        ...
    }
    ...
}

Code Snippets

import com.google.common.collect.ImmutableMap;

private static final Map<String, Pattern> PATTERNS_FOR_TYPE =
    ImmutableMap.of(
    "individual", new IndivualPattern(),
    "multiple", new MultiplePattern(),
    "combined", new CombinedPattern());
private static final Map<String, Pattern> PATTERNS_FOR_DEPARTMENT = ...;

public static Pattern decidePattern(Employee employee) {
    Pattern result = PATTERNS_FOR_TYPE
            .get(employee.getType().toLowerCase(Locale.ENGLISH));
    if (result!=null) {
        return result;
    }
    Pattern result = PATTERNS_FOR_DEPARTMENT
        .get(employee.getDepartment().toLowerCase(Locale.ENGLISH));
    if (result!=null) {
        return result;
    }
    return new DefaultPattern();
}
@RequiredArgsConstructor @Getter 
enum Type {
    INDIVIDUAL(new IndivualPattern()),
    MULTIPLE(new MultiplePattern()),
    COMBINED(new CombinedPattern()),
    ANOTHER(null),
    YET_ANOTHER(null);

    private final Pattern pattern;
}

... enum Department ...

public static Pattern decidePattern(Employee employee) {
    Pattern result = employee.getType().getPattern();
    if (result!=null) {
        return result;
    }
    ...
}
public static Pattern decidePattern(Employee employee) {
    switch (employee.getType()) {
        case INDIVIDUAL: return new new IndivualPattern();
        ...
        default: // (just to prevent "missing label" warning)
    }
    switch (employee.getType()) {
        ...
    }
    ...
}

Context

StackExchange Code Review Q#91901, answer score: 13

Revisions (0)

No revisions yet.