11
\$\begingroup\$

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?

\$\endgroup\$
9
  • \$\begingroup\$ Quick question, what might possibly throw an Exception from the snippet? Getting the type or the department? \$\endgroup\$ – h.j.k. May 27 '15 at 7:20
  • \$\begingroup\$ @h.j.k.. We need to return default pattern when not found or on exception. \$\endgroup\$ – Patan May 27 '15 at 7:37
  • \$\begingroup\$ I understand that, what I'm trying to say is if nothing is throwing an Exception then you wouldn't need the try-catch in the first place. :) \$\endgroup\$ – h.j.k. May 27 '15 at 7:39
  • \$\begingroup\$ @h.j.k. There is a possibility that getType or getDepartment may throw exceptions in my case. \$\endgroup\$ – Patan May 27 '15 at 7:41
  • 2
    \$\begingroup\$ After looking at the answers, I'd say your code is short, straightforward, easy to understand, easy to maintain, so what more do you want! \$\endgroup\$ – gnasher729 May 27 '15 at 14:08

10 Answers 10

13
\$\begingroup\$

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<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();
}

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()) {
        ...
    }
    ...
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Just saying: If you can't use any external libraries (or don't want to use that specific one) you can use a HashMap<String, Pattern>. \$\endgroup\$ – Fund Monica's Lawsuit May 27 '15 at 12:06
  • 3
    \$\begingroup\$ Please do not use toLowerCase without specifying a Locale like toLowerCase(Locale.ENGLISH). "Individual".toLowerCase() is not equal to "individual" in the turkish locale. \$\endgroup\$ – Johnbot May 27 '15 at 13:33
  • 1
    \$\begingroup\$ @Johnbot I know, I just forgot! Thanks, fixed. \$\endgroup\$ – maaartinus May 27 '15 at 13:36
11
\$\begingroup\$

Inspired by this answer.

You could use a Map<String, Pattern>:

Map<String, Pattern> TYPE = new HashMap<>();
Map<String, Pattern> DEPARTMENT = new HashMap<>();
static {
    TYPE.put("individual", new IndividualPattern());
    // etc.
    DEPARTMENT.put("hr", new HrPattern());
    // etc.
}

Then, your method becomes this:

public static Pattern decidePattern(Employee employee) {
    String t = employee.getType().toLowerCase(Locale.ENGLISH),
           d = employee.getDepartment().toLowerCase(Locale.ENGLISH);
    if (TYPE.containsKey(t)) return TYPE.get(t);
    else if (DEPARTMENT.containsKey(d)) return DEPARTMENT.get(d);
    else return new DefaultPattern();
}

(Note: You have to import java.util.Locale.)

However, I think it'd be better to define the pattern for the Employee in the constructor, then supply a method like getPattern(). If this is going in a constructor, you'll want to move the code there, rather than calling a function on a partially-constructed object.

\$\endgroup\$
8
  • \$\begingroup\$ OP is comparing the strings against two sources, employee.getType() and employee.getDepartment(). Do you think your suggestion, which seems to assume a single source, is easy to adapt for OP's case? \$\endgroup\$ – Andriy M May 27 '15 at 13:07
  • \$\begingroup\$ ...Oops. I didn't see that. I'll update it momentarily. \$\endgroup\$ – Fund Monica's Lawsuit May 27 '15 at 14:27
  • \$\begingroup\$ @AndriyM There, that should work. \$\endgroup\$ – Fund Monica's Lawsuit May 27 '15 at 14:42
  • \$\begingroup\$ The platform-default locale is nearly always wrong for this. Imagine employee.getType() returning "INDIVIDUAL" with a server placed in Turkey. \$\endgroup\$ – maaartinus May 28 '15 at 12:30
  • \$\begingroup\$ @maaartinus And what about when a Turkish person tries to use this same code with Turkish names, and .toLowerCase(Locale.ENGLISH) returns something wrong? The user can change their platform's locale easily; I'm pretty sure that can be specified with a command-line argument, actually, though I'm not certain about that. It's rather harder to change the compiled code. \$\endgroup\$ – Fund Monica's Lawsuit May 28 '15 at 12:43
7
\$\begingroup\$

I'd error on the side of readability and go with a switch statement. To me, it would make the code-flow a little more obvious.

public static Pattern decidePattern(Employee employee) {
    Pattern p = null;
    try{
        switch(employee.getType()){
            case "individual":
                p = new IndivualPattern();
                break;
            case "multiple":
                p = new MultiplePattern();
                break;
            case "combined":
                p = new CombinedPattern();
                break;
            default:
                switch(employee.getDepartment()) {
                    case "financial":
                        p = new FinancialPattern();
                        break;
                    case "hr":
                        p = new HrPattern();
                        break;
                    case "facilities":
                        p = new FacilitiesPattern();
                        break;
                    default:
                        p = new DefaultPattern;
                        break;
                }
                break;        
        }
    } catch (Exception e) {
        p = new DefaultPattern();
    }

    return p;
}

Here it seems more obvious that you're comparing string values, and that you've split out to a primary comparison and secondary comparison.

\$\endgroup\$
2
7
\$\begingroup\$

In Java 7+ you can use strings in switch statement, it will make the code a bit more compact.

Alternately you can create a enum for Type (instead of using String) with getPattern method(). Something like:

public enum Type {
  private Pattern pattern;

  private Type(Pattern pattern) {
     this.pattern = pattern
  }

  public Pattern getPattern() {
     return pattern;
  }

  INDIVIDUAL(new IndividualPattern()),
  HR(new HrPattern()),
  ....
}

UPDATE

After reading all other helpful answers and comments I decided to update my answer to hopefully make it more useful.

First of all the original code does the job and easy to understand and in a small application or a script I would write the same code. I would remove try/catch though.

It can be rewritten using two switch statements, though the code needs to handle null values and lower/upper case carefully to remain correct.

In a bigger application it might be useful to use separate classes/enums for Type and Department.

\$\endgroup\$
7
  • \$\begingroup\$ switch won't do, though, because OP is comparing strings against different sources. I'm not sure how you are proposing to use an enum type in OP's case, but if your suggestion is based on the same erroneous premise that the source is the same, it might not work either. \$\endgroup\$ – Andriy M May 27 '15 at 12:56
  • \$\begingroup\$ Also, the code uses equalsIgnoreCase and I doubt that is equivalent to what a switch statement would do. \$\endgroup\$ – gnasher729 May 27 '15 at 14:06
  • \$\begingroup\$ @AndriyM, comparing against different sources points to possible flaw in business logic. Are type = "individual" and department = "hr" mutually exclusive? Does type always set? What are possible values of type? Still the logic from the original post can be implemented with 2 nested switches. \$\endgroup\$ – kostya May 27 '15 at 16:14
  • \$\begingroup\$ @gnasher729, switch on s.toLowerCase() is equivalent to using equalsIgnoreCase \$\endgroup\$ – kostya May 27 '15 at 16:15
  • \$\begingroup\$ @kostya Maybe it is, but you need to specify Locale for this (see the comment below my answer). And I'm sure that toUpperCase is not exactly equivalent, because of "ß".toUpperCase returning "SS" (two chars), while equalsIgnoreCase works char by char. \$\endgroup\$ – maaartinus May 27 '15 at 17:00
5
\$\begingroup\$

With Java 8 you might try to use Lambdas this way:

static class StringPatternHandler {

    private final String string;
    private final Function<Employee, String> f;
    private final Pattern pattern;

    public StringPatternHandler(String string, Function<Employee, String> f, Pattern pattern){
        this.string =string;
        this.f=f;
        this.pattern=pattern;
    }
    // getters omitted
}

Then create a List of those structures:

List<StringPatternHandler> list= new ArrayList<>();
    list.add(new StringPatternHandler("individual", Employee::getType, new IndivualPattern()));
    list.add(new StringPatternHandler("multiple", Employee::getType, new MultiplePattern()));
    list.add(new StringPatternHandler("combined", Employee::getType, new CombinedPattern()));
    list.add(new StringPatternHandler("financial", Employee::getDepartment, new FinancialPattern()));
    list.add(new StringPatternHandler("hr", Employee::getDepartment, new HrPattern()));
    list.add(new StringPatternHandler("facilities", Employee::getDepartment, new FacilitiesPattern()));

Then you can match your filter with the following code:

    return list.stream().filter( x -> x.string.equalsIgnoreCase(x.f.apply(employee)))
                .map(StringPatternHandler::getPattern)
                .findFirst()
                .orElse(new DefaultPattern());
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Note that this treats the patterns as singletons, returning the same instance each time… which may or may not be a good thing. \$\endgroup\$ – 200_success May 28 '15 at 6:54
  • \$\begingroup\$ True, but it may easily be replaced with a Supplier using IndivualPattern::new \$\endgroup\$ – user140547 May 28 '15 at 7:31
3
\$\begingroup\$

As @gnasher729 said, with two function returns to check and using Strings for those return values, what you have is almost as good as it gets.

I think the real reason you are writing code you don't like is that employee.getType() and .getDepartment() return Strings. I would change those two methods to return interfaces (or classes) instead of Strings if I could so that you can leverage Java's type system. Second best is to wrap Employee in your own TypeSafeEmployee:

class TypeSafeEmployee extends Employee {
    private Employee employee;
    TypeSafeEmployee(Employee e) { employee = e; }

    /** Backwards compatibility only - use getRealType() instead. */
    @Deprecated
    String getType() { return employee.getType(); }

    /** Returns a class representing the employee type. */
    Pattern getRealType() {
        try {
            String type = employee.getType();
            return "individual".equalsIgnoreCase(type) ? new IndivualPattern() :
                   "multiple".equalsIgnoreCase(type) ?   new MultiplePattern() :
                   "combined".equalsIgnoreCase(type) ?   new CombinedPattern() :
                   new DefaultPattern()
        } catch (Exception ignore) {
            return new DefaultPattern();
        }
    }
    /** Backwards compatibility only - use getRealDepartment() instead. */
    @Deprecated
    String getDepartment() { return employee.getDepartment(); }

    /** Returns a class representing the employee department. */
    Pattern getRealDepartment() {
        try {
            String dept = employee.getDepartment();
            return "financial".equalsIgnoreCase(dept) ?  new FinancialPattern() :
                   "hr".equalsIgnoreCase(dept) ?         new HrPattern() :
                   "facilities".equalsIgnoreCase(dept) ? new FacilitiesPattern() :
                   new DefaultPattern()
        } catch (Exception ignore) {
            return new DefaultPattern();
        }
    }
}

Now you can also return different interfaces for the EmployeeTypePattern and DepartmentPattern if you wish. Java doesn't have pattern matching (well it does, but it's referring to Regular Expressions, not to type deconstruction). You can sort-of build your own one-off pattern-matches in Java8.

interface Pattern { ... }
class DefaultPattern implements Pattern { ... }

interface EmployeeTypePattern extends Pattern {
    /** Low-budget pattern matching in Java */
    static <R> R patMatch(TypeSafeEmployee emp,
                          Function<IndividualPattern,R> fi,
                          Function<MultiplePattern,R> fm,
                          Function<CombinedPattern,R> fc,
                          Function<DefaultPattern,R> fd) {
        EmployeePattern ep = emp.getRealType()
        if (ep instanceOf IndividualPattern) {
            return fi.apply((IndividualPattern) ep);
        }
        if (ep instanceOf MultiplePattern) {
            return fm.apply((MultiplePattern) ep);
        }
        if (ep instanceOf CombinedPattern) {
            return fc.apply((CombinedPattern) ep);
        }
        if (ep instanceOf DefaultPattern) {
            return fd.apply((DefaultPattern) ep);
        }
        throw new IllegalStateException(
                "Hey - who added a new kind of EmployeePattern?");
    }
}
class IndividualPattern implements EmployeeTypePattern { ... }
...

interface DepartmentPattern extends Pattern { ... }
class FinancialPattern implements DepartmentPattern { ... }
...

Instead of classes to implement your interfaces, you could use enums as others suggested if that's appropriate.

\$\endgroup\$
3
\$\begingroup\$

In Java 8 some lambdas would indeed be the way to go. You can get some really clean and readable code with those. A big issue with the sample code is, however, that there are strings that are used as enums. Those must be converted to real enums.

My solution uses a list of Patterns that are ordered by priority, and there's a new helper class called PatternWrapper which has a method that tells whether an employee matches that pattern.

First the imports:

import java.util.List;
import java.util.function.BiPredicate;
import java.util.function.Supplier;
import com.google.common.collect.ImmutableList;

Then, we shall assume the department and employer type are turned to enums like this -- this is left as an exercise to the reader:

  public enum Type { INDIVIDUAL, MULTIPLE, COMBINED }
  public enum Department { FINANCIAL, HR, FACILITIES }

Then a new helper class:

  public static class PatternWrapper {
    private final BiPredicate<Type, Department> predicate;
    private final Supplier<Pattern> supplier;

    public PatternWrapper(final BiPredicate<Type, Department> predicate, Supplier<Pattern> patternSupplier) {
      this.predicate = predicate;
      supplier = patternSupplier;
    }

    public boolean accept(Employee e) {
      return this.predicate.test(e.type, e.department);
    }

    public Pattern getPattern() {
      return this.supplier.get();
    }
  }

And finally the beef of the solution:

  private static final List<PatternWrapper> list = ImmutableList.of(
      new PatternWrapper((t, d) -> t == Type.INDIVIDUAL, IndivualPattern::new),
      new PatternWrapper((t, d) -> t == Type.MULTIPLE, MultiplePattern::new),
      new PatternWrapper((t, d) -> t == Type.COMBINED, CombinedPattern::new),
      new PatternWrapper((t, d) -> d == Department.FINANCIAL, FinancialPattern::new),
      new PatternWrapper((t, d) -> d == Department.HR, HrPattern::new),
      new PatternWrapper((t, d) -> d == Department.FACILITIES, FacilitiesPattern::new)
  );

  public static Pattern decidePattern(Employee employee) {
    return list.stream()
        .filter(pw -> pw.accept(employee))
        .findFirst()
        .map(pw -> pw.getPattern())
        .orElse(new DefaultPattern());
  }
}

This improves over this solution in that the wrapper is typed in a meaningful way, instead of using just strings. Additionally, the pattern matching logic is in the wrapper class, making the stream code in the decidePattern method generic and easy to read. The Patterns are also provided with Suppliers instead of always returning the same instance.

If you'd later have to match the employees by other qualities besides their type and department, such as position, you'd change the PatternWrapper to contain just a Predicate<Employee> instead of a BiPredicate<Type, Department>. However, doing too generic code prematurely is not good, so for now I'd stick to the BiPredicate.

I have to admit I'm a bit upset by the wrapper class which should be unnecessary, but unfortunately we don't have tuples in Java by default. Another way would be to replace the List with a LinkedHashMap which also holds the order, but then you'd be streaming its entrySet, which I find a bit smelly. At least the wrapper class can now provide the clean accept(Employee) method.

\$\endgroup\$
2
  • \$\begingroup\$ This is just my personal opinion, but I find this code much less readable the the original version. I believe there is no need to use lambdas where a simple if would do the job. \$\endgroup\$ – kostya May 29 '15 at 5:35
  • 1
    \$\begingroup\$ Oh, but it was proven several times already that the original version is NOT readable. About half of the people who answered to this question missed the fact that the pattern is chosen according to two parameters, not just one. My solution makes that obvious. \$\endgroup\$ – ZeroOne May 29 '15 at 5:55
2
\$\begingroup\$

First of all, you shouldn't catch Exception. You should catch the most specific type of exception that might be thrown. The main reason for this is to avoid masking exceptions that would be truly unexpected. A second reason is to make it easier to review the code, by making it clear what can go wrong.

Secondly, don't put large blocks of code in a try-catch. The main reason for this is to avoid masking exceptions that might get thrown at a different point within the block that you expected. A second reason is that the graceful recovery typically depends on the point of the error, so you need to know where exactly it was.

In your comment you said that getType() or getDepartment() might throw, we'll get to cleaning that up soon.

Another thing I don't like in this code is the repeated calls to employee.getType() and employee.getDepartment().

To clean up both of these issues, I'd rewrite like this:

    String type;
    try {
        type = employee.getType().toLowerCase(Locale.ENGLISH);
    } catch (EmployeeTypeException e) {
        return new DefaultPattern();
    }
    if ("individual".equals(type)) {
        return new IndivualPattern();
    }
    if ("multiple".equals(type)) {
        return new MultiplePattern();
    }
    if ("combined".equals(type)) {
        return new CombinedPattern();
    }

    String department;
    try {
        department = employee.getDepartment.toLowerCase(Locale.ENGLISH);
    } catch (EmployeeDepartmentException e) {
        return new DefaultPattern();
    }
    if ("financial".equals(department)) {
        return new FinancialPattern();
    }
    if ("hr".equals(department)) {
        return new HrPattern();
    }
    if ("facilities".equals(department)) {
        return new FacilitiesPattern();
    }
    return new DefaultPattern();

Instead of EmployeeTypeException and EmployeeDepartmentException use the appropriate exceptions that might be thrown there.

Some other improvements I slipped in there:

  • No repeated equalsIgnoreCase. Convert to lowercase once, then use simple and more efficient equals
  • No need for the else if if the previous branch returns anyway

Some more improvements are possible:

  • Instead of hardcoded strings like "hr", "facilities", there are better options:
    • Use constants (static final fields)
    • Use enums
    • Instead of the if conditions, use a Map
  • Do you need new pattern instances on every call? If possible, it would be good to store those instances in static final fields and reuse them rather than creating every time
\$\endgroup\$
2
\$\begingroup\$

You can refactor your code with enum factory

public enum PatternFactory {
    INDIVUAL {
        @Override
        public Pattern create() { return new IndivualPattern(); }
    },
    MULTIPLE {
        @Override
        public Pattern create() { return new MultiplePattern(); }
    },

    ...

    /**
      * Override this method in enum value declaration
      */
    public Pattern create() { return null; /* Or throw an exception */ }

    public static PatternFactory valueOfIgnoreCase(String name) {
        for (PatternFactory factory : values()) {
            if (factory.name().equalsIgnoreCase(name)) {
                return factory;
            }
        }
        return PatternFactory.DEFAULT;
    }
}
...
PatternFactory factory = PatternFactory.valueOfIgnoreCase(employee.getType());
if (factory == PatternFactory.DEFAULT) {
    factory = PatternFactory.valueOfIgnoreCase(employee.getDescription());
}
return factory.create();
\$\endgroup\$
2
  • \$\begingroup\$ I kind of like this one, but you missed the fact that the strings are compared against two sources: employee.getType() and employee.getDepartment(). How does your solution adapt to that? \$\endgroup\$ – ZeroOne May 27 '15 at 14:07
  • \$\begingroup\$ @ZeroOne well... The only way I can think of is by leaving one last if statement to check whether PatternFactory was successfuly determined on first try or not. If not, just run valueOfIgnoreCase again for next value. As I see it every solution will still have this last condition so its not a big issue. P.S. I've updated my answer \$\endgroup\$ – SimY4 May 27 '15 at 14:58
0
\$\begingroup\$

All of your patterns have reasonable names, except IndivualPattern. Weird abbreviations like that will frustrate programmers.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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