patternjavaModerate
Picking a pattern based on employee type or department
Viewed 0 times
employeepickingtypebaseddepartmentpattern
Problem
I have an
The
All patterns are inherited from common interface. Can I break this into a simpler one?
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
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
In case you insist on
Concerning
You can (and most probably should) use
Not always it is a good idea to define the pattern in the
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 likeimport 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 switchpublic 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.