Problem
Given below is an exercise from the book *Java SE 8 for the Really Impatient( by Cay S. Horstmann:
Write a method that generates a Comparator that can be normal or
reversed, case-sensitive or case-insensitive, space-sensitive or
space-insensitive, or any combination thereof. Your method should
return a lambda expression.
Is there a better (efficient, elegant) way to do it than below?
public static final int COMPARE_REVERSED = 0x01;
public static final int COMPARE_CASE_INSENSITIVE = 0x02;
public static final int COMPARE_SPACE_INSENSITIVE = 0x04;
public static Comparator<String> getComparator(int flags) {
return (s1, s2) -> {
if (has(flags, COMPARE_REVERSED)) {
String temp = s1;
s1 = s2;
s2 = temp;
}
if (has(flags, COMPARE_SPACE_INSENSITIVE)) {
s1 = s1.replaceAll("\s", "");
s2 = s2.replaceAll("\s", "");
}
if (has(flags, COMPARE_CASE_INSENSITIVE)) {
return s1.compareToIgnoreCase(s2);
}
return s1.compareTo(s2);
};
}
public static boolean has(int flags, int FLAG) {
return (flags & FLAG) != 0;
}
Solution
This seems as efficient as it gets, to me.
In terms of elegance, Joshua Bloch (in Effective Java) recommends to stop using bit fields, use EnumSet instead.
The first step is to change your bit fields to an enum:
public enum Mode {
COMPARE_REVERSED,
COMPARE_CASE_INSENSITIVE,
COMPARE_SPACE_INSENSITIVE
}
Next, change your method to take a Set instead of an int:
public static Comparator<String> getComparator(Set<Mode> flags) {
return (s1, s2) -> {
if (flags.contains(Mode.COMPARE_REVERSED)) {
String temp = s1;
s1 = s2;
s2 = temp;
}
if (flags.contains(Mode.COMPARE_SPACE_INSENSITIVE)) {
s1 = s1.replaceAll("\s", "");
s2 = s2.replaceAll("\s", "");
}
if (flags.contains(Mode.COMPARE_CASE_INSENSITIVE)) {
return s1.compareToIgnoreCase(s2);
}
return s1.compareTo(s2);
};
}
You could call this method with any Set, but it will be best to call with an EnumSet, for example:
getComparator(EnumSet.of(Mode.COMPARE_CASE_INSENSITIVE, Mode.COMPARE_SPACE_INSENSITIVE));
Why is this better?
- You don’t need to worry about setting the bit fields correctly to be mutually exclusive
- You don’t need the
hasmethod anymore, now you can use simpleSet.contains - You don’t need the
... | ...bit operations anymore,EnumSet.of(...)is simple and clean
In short (quoted from the book):
No more bit twiddling,
EnumSetdoes all the hard work for you.
I would also add a convenience method for the case of no flags:
public static Comparator<String> getComparator() {
return getComparator(Collections.<Mode>emptySet());
}
And of course, a bunch of unit tests, for example:
@Test
public void testNormalComparator() {
Comparator<String> comparator = ComparatorGenerator.getComparator();
assertNotEquals(0, comparator.compare("hello", "Hello"));
assertEquals(0, comparator.compare("hello", "hello"));
}
@Test
public void testCaseInsensitiveComparator() {
Comparator<String> comparator = ComparatorGenerator.getComparator(EnumSet.of(ComparatorGenerator.Mode.COMPARE_CASE_INSENSITIVE));
assertNotEquals(0, comparator.compare("hello", "xHello"));
assertEquals(0, comparator.compare("hello", "Hello"));
assertEquals(0, comparator.compare("hello", "hello"));
}
@Test
public void testReversedComparator() {
Comparator<String> comparator1 = ComparatorGenerator.getComparator();
assertEquals(-1, comparator1.compare("hello1", "hello2"));
Comparator<String> comparator2 = ComparatorGenerator.getComparator(EnumSet.of(ComparatorGenerator.Mode.COMPARE_REVERSED));
assertEquals(1, comparator2.compare("hello1", "hello2"));
}
@Test
public void testCaseAndSpaceInsensitiveComparator() {
Comparator<String> comparator = ComparatorGenerator.getComparator(EnumSet.of(ComparatorGenerator.Mode.COMPARE_CASE_INSENSITIVE, ComparatorGenerator.Mode.COMPARE_SPACE_INSENSITIVE));
assertNotEquals(0, comparator.compare("hello there", "x hello there"));
assertEquals(0, comparator.compare("HELLO there", "h e llo there"));
}
Your answer is already pretty good, and after @janos’s refactoring using EnumSet you would/should probably stop IRL.
But what is an improvement (optimization) depends on your values (loss function). if you resolve to “extract till you drop”, you can find a few more refactorings to be made.
- One thing that isn’t DRY is
if (has(flags, someFlag)) {do something};snippets. - those snippets are not reified (they are not explicit methods/variables etc)
- The flag and the snippet it affects do not have an explicit relation.
- the parameters are assigned.
I started with extracting the bodies of if statements to static methods of signature Comparator<String> spaceInsensitive(Comparator<String> c).
I then converted these methods to constants of type Function<Comparator<String>, Comparator<String>>.
And moved these constants into the @janos’s Mode enum, so that the I have a 1-to-1 mapping between the option and the transformation it does:
public enum Mode {
REVERSED(c -> c.reversed()),
CASE_INSENSITIVE(c -> (s1, s2) -> c.compare(
s1.toLowerCase(Locale.ROOT),
s2.toLowerCase(Locale.ROOT))),
SPACE_INSENSITIVE(c -> (s1, s2) -> c.compare(
s1.replaceAll("\s", ""),
s2.replaceAll("\s", "")));
public final Function<Comparator<String>, Comparator<String>> transform;
Mode(Function<Comparator<String>, Comparator<String>> transform) {
this.transform = transform;
}
}
Extracting the repetitive parts of CASE_INSENSITIVE and SPACE_INSENSITIVE, we get, (becoming a Three-Arrow-Java-Programmer as a side effect) :
public enum Mode {
REVERSED(c -> c.reversed()),
CASE_INSENSITIVE(transformingEachParam.apply(s -> s.toLowerCase(Locale.ROOT))),
SPACE_INSENSITIVE(transformingEachParam.apply(s -> s.replaceAll("\s", "")));
public final Function<Comparator<String>, Comparator<String>> transform;
Mode(Function<Comparator<String>, Comparator<String>> transform) {
this.transform = transform;
}
}
private static final Function<Function<String, String>, Function<Comparator<String>, Comparator<String>>>
transformingEachParam = f -> c -> (s1, s2) -> c.compare(f.apply(s1), f.apply(s2));
Changing ifs to ternary operators, for considerations of composability:
public static Comparator<String> getComparator(Set<Mode> flags) {
Comparator<String> result = Comparator.naturalOrder();
result = flags.contains(Mode.SPACE_INSENSITIVE)
? Mode.SPACE_INSENSITIVE.transform.apply(result)
: result;
// ... the same for other options ....
return result;
}
It is now trivial to roll these into a for each loop:
public static Comparator<String> getComparator(Set<Mode> flags) {
Comparator<String> result = Comparator.naturalOrder();
for (Mode mode : Mode.values()) {
result = flags.contains(mode)
? mode.transform.apply(result)
: result;
}
return result;
}
This is another good place to stop, but since we are learning Java 8 let’s convert this loop to Stream API. Stream API does not want us (It is an opinionated API) to write reductions as “start with this value, keep modifying it in that way”. It wants us to use associative and stateless binary operations to combine elements of a stream. Luckily for us function composition is both associative and stateless.
Final form:
public static Comparator<String> getComparator2(Set<Mode> flags) {
Comparator<String> result = flags.stream()
.map(m -> m.transform)
.reduce(Function.identity(), (f1, f2) -> f1.andThen(f2))
.apply(Comparator.<String>naturalOrder());
// to fulfill "return a lambda expression" requirement ;)
return (s1, s2) -> result.compare(s1, s2);
}