I’ve done a few posts on the Hamcrest library, and I really do enjoy using it, but there are a few changes I would love to make to it. I understand most of the design decisions that they made, but I think some of them weren’t really worth it.
Introducing Litecrest
Most of the changes I would make to the library help to lighten the load of Hamcrest, since I feel like there are a few things that weigh it down unnecessarily. This is why I call my changes Litecrest. It won’t be an actual library; this is all just thinking aloud. I also hope that you’ll learn a little about designing libraries from this.
No Descriptions
The Description interface and StringDescription and BaseDescription classes aren’t really worthwhile. They provide some nice methods for converting lists to nice Strings, but the toString() method on all of those should be sufficient. If not, one could put some protected final methods on the BaseMatcher to use for conveniently building Strings for lists. Granted, this doesn’t really follow SRP that closely, so you could use something like Description to provide the convenience methods.
Description, otherwise, isn’t very helpful. Its very presence supposes that it’s there specifically to provide an output that may not be a String in the long run. Being a well-used library, changing it from String to a output-agnostic type would break backwards compatibility down the road, but such a change isn’t likely to be needed. Apply YAGNI, and the Description class goes right down the toilet.
No Out Parameters
The describeTo() and describeMismatch should not be taking in a Description or any other type of String appending object, especially as an out parameter (something to avoid as often as possible). Seeing as those methods don’t have a return type to begin with, there’s definitely no reason to use an out parameter.
Looking at the problem a little closer, you’ll see there’s no reason for a parameter at all. I understand that they may have been trying to force the creators of matchers to not use String concatenation, but that shouldn’t be. If a matcher’s description was just a simple little String, there’s no reason why they shouldn’t be able to just return that String. Personally, I would have removed the Description parameters and given them a return type of String or CharSequence. I consider CharSequence because then it gives a higher incentive to use StringBuilder, but simply returning a String is no big deal either, since they can call toString() on it. I probably would go with CharSequence, though, too, since I’d be using a StringBuilder in the assertion logic to put together the output, and StringBuilders can take in CharSequences too, so the only toString() that would ever have to be called is when finalizing the output.
Type-Safety
The Matcher interface takes in a generic parameter, which is meant to go with the matches() method, but said method takes in an Object instead of the generic type. The javadoc claims that this is because of type erasure, but I don’t see how that’s a problem. I haven’t done any digging to try out whether you could switch it over to the generic type, but if I found that you actually could use the generic type, I would. This eliminates the need for the TypeSafeMatcher, which, because it also checks for null, could be replaced with a simpler NullCheckingMatcher, or just implement it so that the assertion will change the mismatch description to “was null” if it catches a NullPointerException. By doing all of this, we can possibly eliminate all the other base classes that had to be doubled up just to cover the type-safe matchers and matchers that are less so. (examples: CustomMatcher and CustomTypeSafeMatcher, DiagnosingMatcher and TypeSafeDiagnosingMatcher, and my doubled-up ChainableMatchers – heck, get rid of both DiagnosingMatchers; they’re a poor design, calling matches() twice)
Change Some Names
I really don’t like the name describeTo(). It should be describeExpected() or describeMatch(). I understand that they were following the naming convention of SelfDescribing in the JMock Constraints, but seeing as they didn’t bother to finish copying the rest of the method signature, it doesn’t really do any good.
CustomMatchers should be called OneOffMatchers or QuickMatchers. Custom is a misleading name, making it sound like you need to extend from it in order to even make your own matchers.
More Examples in Documentation
There are a few classes in the library that I’m not sure how useful they are because their documentation doesn’t show how they’re used. Condition is one of those. From the little bit of documentation, it sounds like it would be relatively useful, but since it provides no examples of use (and it’s a relatively complex file with an inner interface and two inner classes), I have no idea how to use it. It also doesn’t document its public methods, so I’m not sure what they do without a lot of digging.
FeatureMatcher is decently documented, but again, there are no examples.
Those writing documentation for a library much keep that in mind at all times; if it’s not totally obvious (often, even if it is), you should give examples of your class in use.
Remove Extraneous Classes
Some of these have already been gone over, whether directly or indirectly. Remove Description and all of its subclasses. Remove SelfDescribing, since it’s really only useful if Description still exists. Remove all the TypeSafe versions of base matchers. Remove the Diagnosing matchers. I’m not sure if I should remove Condition because I don’t how useful it is. If we keep Condition, then we end up with five of the original eleven classes in the core org.hamcrest package and two of the original four interfaces in the api org.hamcrest package.
Now let’s dig into org.hamcrest.internal package. ArrayIterator isn’t useful since you can just use arrays can already be used with a foreach loop. NullSafety seems to mimic Arrays.toList() functionality, but replaces null matchers with the IsNull matcher. I don’t see how this is helpful, so I’ll remove it. ReflectiveTypeFinder may end up being useful. I’ve only seen it used in TypeSafeMatcher and FeatureMatcher, though I’m not sure how much it’s used in FeatureMatcher. I’ll keep it, though. The last two deal with SelfDescribing, which we’ve removed, so these two go as well. That only leaves ReflectiveTypeFinder from the five classes that used to be here.
I’m not going to go into the all the other matchers; for the most part, they’ve been added for their usefulness. There would likely have to be changes to almost all of them due to the removal of so many of the base classes.
Lambdas!
You could expand the usefulness of the matcher idea if you applied the new functional paradigm to hamcrest as well. I haven’t thought of much, but for one-off matchers, you could modify the library to include a new assertThat() method that looks like this:
public static void assertThat(T item, String description, Predicate matcher) {
if(!matcher.test(item)) {
StringBuilder output = new StringBuilder();
output.append("Expected: ")
.append(description)
.append("\n but: was")
.append(item.toString());
throw new AssertionError(output.toString());
}
}This would allow you to write assertions similar to:
assertThat("cats", "doesn't contain \"dogs\"", str -> !str.contains("dogs"));In fact, I’ve actually added a LambdaAssert class to my ez-testing mini library, so you could use this with the original hamcrest library.
Matcher Interface
There is a Matcher interface that is essentially pointless because hamcrest wants you to extend BaseMatcher rather than implementing Matcher. Why would you create an interface if you very strictly don’t want anyone to implement? Especially since the only thing that BaseMatcher does for us is create a default implementation for describeMismatch() (that, and “implement” the deprecated method that was put there to tell you to use BaseMatcher instead of Matcher).
If you really really don’t want people using the interface, then get rid of it. Personally, since I often override describeMismatch() anyway, I feel that it should be totally okay to simply implement the interface, instead of having to make the JVM load a base class that actually provides nothing for me.
Plus, since we have Java 8 now, the interface could just use a default method to make the default implementation. I can understand wanting to avoid this, though, since older versions of Java would not be able to utilize this.
So, either just make BaseMatcher or be okay with Matcher being implemented.
Outro
There are other little things that I would like to change, such as forcing people to override describeMismatch() instead of providing a default, but I’m not even certain about that one, since the default would generally be effective enough. Anyway, even if you have a popular library, it doesn’t mean that it’s perfect. Always be on the lookout for refactoring you can do.
Unfortunately, all of these changes would not be backwards-compatible, but sometimes it’s worth it.
| Reference: | Redesigning Hamcrest from our JCG partner Jacob Zimmerman at the Programming Ideas With Jake blog. |
