27

I was just looking at the Java Hamcrest code on GitHub, and noticed they employed a strategy that seemed unintuitive and awkward, but it got me wondering if I'm missing something.

I noticed in the HamCrest API that there is an interface Matcher and an abstract class BaseMatcher. The Matcher interface declares this method, with this javadoc:

    /**
     * This method simply acts a friendly reminder not to implement Matcher directly and
     * instead extend BaseMatcher. It's easy to ignore JavaDoc, but a bit harder to ignore
     * compile errors .
     *
     * @see Matcher for reasons why.
     * @see BaseMatcher
     * @deprecated to make
     */
    @Deprecated
    void _dont_implement_Matcher___instead_extend_BaseMatcher_();

Then in BaseMatcher, this method is implemented as follows:

    /**
     * @see Matcher#_dont_implement_Matcher___instead_extend_BaseMatcher_()
     */
    @Override
    @Deprecated
    public final void _dont_implement_Matcher___instead_extend_BaseMatcher_() {
        // See Matcher interface for an explanation of this method.
    }

Admittedly, this is both effective and cute (and incredibly awkward). But if the intention is for every class that implements Matcher to also extend BaseMatcher, why use an interface at all? Why not just make Matcher an abstract class in the first place and have all other matchers extend it? Is there some advantage to doing it the way Hamcrest has done it? Or is this a great example of bad practice?

EDIT

Some good answers, but in search of more detail I'm offering a bounty. I think that the issue of backwards / binary compatibility is the best answer. However, I'd like to see the issue of compatibility elaborated on more, ideally with some code examples (preferably in Java). Also, is there a nuance between "backwards" compatibility and "binary" compatibility?

FURTHER EDIT

January 7, 2014 -- pigroxalot provided an answer below, linking to this comment on Reddit by the authors of HamCrest. I encourage everyone to read it, and if you find it informative, upvote pigroxalot's answer.

EVEN FURTHER EDIT

December 12, 2017 -- pigroxalot's answer was removed somehow, not sure how that happened. It's too bad... that simple link was very informative.

James Dunn
  • 8,064
  • 13
  • 53
  • 87
  • 1
    I suppose if they change BaseMatcher in the future, they won't break code that is dependent on use of Matcher type objects. I also wonder if it helps with dependency injection (just a wild guess as I'm no pro). – Hovercraft Full Of Eels Dec 20 '13 at 22:18
  • Java (along with most modern OO languages) disallows multiple inheritance, so Hamcrest seems to be saying "we don't want any other kind of class to implement this interface" while still making it an interface instead of an abstract class. Why they have done that is a mystery to me. – Brian A. Henning Dec 20 '13 at 22:19
  • 1
    If they didn't want anyone to implement an interface they should have made it `internal`. That looks like a horrible hack from someone who doesn't know how to program correctly. – Federico Berasategui Dec 20 '13 at 22:21
  • 10
    Frankly, I think you have an example here of poor practice, and if you want to know why they did it, you'll have to ask them directly. – T.J. Crowder Dec 20 '13 at 22:21
  • 1
    @HighCore there's no `internal` in Java; you must be thinking of C#. – ajb Dec 20 '13 at 22:25
  • The only possible advantage I can think of is that the interface grants you the ability to use Java proxies and the abstract class can hold logic that is needed by all Matchers. Not sure if they use proxies under the hood though. This seems strange to me, I'm very curious now – jeff Dec 20 '13 at 22:26
  • @ajb I know that, java seems to have a [crappy substitution](http://stackoverflow.com/questions/5981107/is-there-anything-like-an-internal-class-in-java), though, just like with every other C# feature. – Federico Berasategui Dec 20 '13 at 22:31
  • 5
    That looks like a way to maintain backward binary compatibility with previous implementations, implementing Matcher directly, but to ensure that nobody does it anymore by breaking source-code compatibility. – JB Nizet Dec 20 '13 at 22:32
  • @JBNizet I wonder if you could elaborate more on binary compatibility? It seems that by adding the "cute" method, previous implementations would break anyway. – James Dunn Dec 20 '13 at 22:43
  • 3
    No. The class would be loaded fine. It would only cause a runtime error (NoSuchMoethodError, IIRC) if the absent method was called. That's what allows old JDBC drivers to still work fine on recent JREs, although a whole lot of methods have been added to Connection, Statement, ResultSet, etc. If you don't call the unimplemented methods, no problem. – JB Nizet Dec 20 '13 at 22:47
  • @JBNizet the new methods *should* throw SQLException by default, which is much better than an `Error`. – ZhongYu Dec 20 '13 at 22:58
  • 2
    @zhong.j.yu They can't throw a SQLException by default, since they're methods added to interfaces, and methods in interfaces (until Java 8) can't have any default implementation. – JB Nizet Dec 20 '13 at 23:01
  • @JBNizet yes, but they can/should do it now, see `Iterator.remove()` – ZhongYu Dec 20 '13 at 23:19
  • @zhong.j.yu Java 8 isn't widely-enough deployed yet for JDBC driver authors to be able to rely on it. – Donal Fellows Dec 21 '13 at 07:12
  • Great question, insightful discussion. So weird yet interesting... – Jake Toronto Oct 03 '14 at 16:22
  • What happened to @pigroxalot's answer? – Jake Toronto Oct 03 '14 at 16:28
  • Either @pigroxalot or someone else deleted it, which is sad -- it was a good answer and I would have awarded it my bounty had he answered in time. I'm sure glad I updated my question to include the link in his answer. – James Dunn Oct 03 '14 at 18:08

7 Answers7

10

The git log has this entry, from December 2006 (about 9 months after the initial checkin):

Added abstract BaseMatcher class that all Matchers should extend. This allows for future API compatability [sic] as the Matcher interface evolves.

I haven't tried to figure out the details. But maintaining compatibility and continuity as a system evolves is a difficult problem. It does mean that sometimes you end up with a design that you would never, ever, ever have created if you had designed the whole thing from scratch.

ajb
  • 31,309
  • 3
  • 58
  • 84
  • 1
    Summarizing the additional functionality provided by the abstract class would make this good answer great. – David Harkness Dec 22 '13 at 18:42
  • @DavidHarkness agreed. Hence, the bounty. – James Dunn Dec 23 '13 at 21:47
  • Marking this answer as accepted, because it was the first given that, in my opinion, is correct. I do wish that it elaborated in more detail. – James Dunn Dec 29 '13 at 05:40
  • @TJamesBoone I think you'd have to ask one of the Hamcrest developers for more detail. I was able to find the `git log` entry, and I thought it would be helpful, but I really don't know anything about the internals. – ajb Dec 29 '13 at 19:34
5

But if the intention is for every class that implements Matcher to also extend BaseMatcher, why use an interface at all?

It's not exactly the intent. Abstract base classes and interfaces provide entirely different 'contracts' from an OOP perspective.

An interface is a communication contract. An interface is implemented by a class to signify to the world that it adheres to certain communication standards, and will give a specific type of result in response to a specific call with specific parameters.

An abstract base class is an implementation contract. An abstract base classes is inherited by a class to provide functionality that is required by the base class but left for the implementer to provide.

In this case, both overlap, but this is merely a matter of convenience - the interface is what you need to implement, and the abstract class is there to make implementing the interface easier - there is no requirement whatsoever to use that base class to be able to offer the interface, it's just there to make it less work to do so. You are in no way limited in extending the base class for your own ends, not caring about the interface contract, or in implementing a custom class implementing the same interface.

The given practice is actually rather common in old-school COM/OLE code, and other frameworks facilitating inter-process communications (IPC), where it becomes fundamental to separate implementation from interface - which is exactly what is done here.

Niels Keurentjes
  • 41,402
  • 9
  • 98
  • 136
  • 1
    "It's not exactly the intent"... I disagree. I agree with most of your answer, but I think Hamcrest has made it pretty clear that they **don't** want anyone to implement the interface without also extending the abstract class (even though it is still technically possible). Otherwise, why bother writing an awkward and functionally useless method that will clutter up their API? This is beyond "a matter of convenience". If that were all that it was, they could have just noted the abstract class in the javadocs of the interface. – James Dunn Dec 23 '13 at 15:36
3

I think what happened is that initially a Matcher API was created in the form of an interface.
Then while implementing the interface in various ways a common code base was discovered which was then refactored out into the BaseMatcher class.

So my guess is that the Matcher interface was retained as it is part of the initial API and the descriptive method was then added as a reminder.

Having searched through the code, I found that the interface could easily by done away with as it is ONLY implemented by BaseMatcher and in 2 test units which could easily be changed to use BaseMatcher.

So to answer your question - in this particular case there is no advantage to doing it this way, besides not breaking other peoples implementations of Matcher.

As to bad practice ? In my opinion it is clear and effective - so no I don't think so, just a little odd :-)

Jurgen
  • 2,138
  • 12
  • 18
  • Although this answer didn't go into all the detail I was looking for (see my edit at the bottom of my question), I feel that it is closest to deserving the bounty. Hence, I award the bounty to this answer. – James Dunn Dec 29 '13 at 05:43
3

Hamcrest provides matching, and matching only. It is a tiny niche market but they appear to be doing it well. Implementations of this Matcher interface are littered across a couple unit testing libraries, take for example Mockito's ArgumentMatcher and across a silly great amount of tiny anonymous copy-paste implementations in unit tests.

They want to be able to extend the Matcher with a new method without breaking all of those existing implementing classes. They would be hell to upgrade. Just imagine suddenly having all your unittest classes showing angry red compile errors. The anger and annoyance would kill hamcrest's niche market in one quick swoop. See http://code.google.com/p/hamcrest/issues/detail?id=83 for a small taste of that. Also, a breaking change in hamcrest would divide all versions of libraries that use Hamcrest into before and after the change and make them mutually exclusive. Again, a hellish scenario. So, to keep some freedom, they need Matcher to be an abstract base class.

But they are also in the mocking business, and interfaces are way easier to mock than base classes. When the Mockito folks unit test Mockito, they should be able to mock the matcher. So they also need that abstract base class to have a Matcher interface.

I think they have seriously considered the options and found this to be the least bad alternative.

Community
  • 1
  • 1
flup
  • 26,937
  • 7
  • 52
  • 74
2

There is an interesting discussion about it here. To quote nat_pryce:

Hi. I wrote the original version of Hamcrest, although Joe Walnes added this wierd method to the base class.

The reason is because of a peculiarity of the Java language. As a commenter below said, defining Matcher as a base class would make it easier to extend the library without breaking clients. Adding a method to an interface stops any implementing classes in client code from compiling, but new concrete methods can be added to an abstract base class without breaking subclasses.

However, there are features of Java that only work with interfaces, in particular java.lang.reflect.Proxy.

Therefore, we defined the Matcher interface so that people could write dynamic implementations of Matcher. And we provided the base class for people to extend in their own code so that their code would not break as we added more methods to the interface.

We have since added the describeMismatch method to the Matcher interface and client code inherited a default implementation without breaking. We also provided additional base classes that make it easier to implement describeMismatch without duplicating logic.

So, this is an example of why you can't blindly follow some generic "best practice" when it comes to design. You have to understand the tools you're using and make engineering trade-offs within that context.

EDIT: separating the interface from the base class also helps one cope with the fragile base class problem:

If you add methods to an interface that is implemented by an abstract base class, you may end up with duplicated logic either in the base class or in subclasses when they are changed to implement the new method. You cannot change the base class to remove that duplicated logic if doing so changes the API provided to subclasses, because that will break all subclasses -- not a big problem if the interface and implementations are all in the same codebase but bad news if you're a library author.

If the interface is separate from the abstract base class -- that is, if you distinguish between users of the type and implementers of the type -- when you add methods to the interface you can add a default implementation to the base class that will not break existing subclasses and introduce a new base class that provides a better partial implementation for new subclasses. When someone comes to change existing subclasses to implement the method in a better way, then can choose to use the new base class to reduce duplicated logic if it makes sense to do so.

If the interface and base class are the same type (as some have suggested in this thread), and you then want to introduce multiple base classes in this way, you're stuck. You can't introduce a new supertype to act as the interface, because that will break client code. You can't move the partial implementation down the type hierarchy into a new abstract base class, because that will break existing subclasses.

This applies as much to traits as Java-style interfaces and classes or C++ multiple inheritance.

eis
  • 51,991
  • 13
  • 150
  • 199
pigroxalot
  • 100
  • 3
  • Thank you! That article helped me understand it much more clearly than I had before. This is what I was looking for! – James Dunn Jan 07 '14 at 17:44
1

Java8 now allows new methods to be added to an interface if they contains default implementations.

interface Match<T>

    default void newMethod(){ impl... }

this is a great tool that gives us a lot of freedom in interface design and evolution.

However, what if you really really want to add an abstract method that has no default implementation?

I think you should just go ahead and add the method. It'll break some existing codes; and they will have to be fixed. Not really a big deal. It probably beats other workarounds that preserve binary compatibility at the cost of screwing up the whole design.

ZhongYu
  • 19,446
  • 5
  • 33
  • 61
  • 1
    Can anyone explain to me how this "feature" of adding code in interfaces is an improvement? It looks to me like a horrible HACK which violates the very nature of OOP by introducing executable code in interfaces, which conceptually should be `Contracts` and contain NO code – Federico Berasategui Dec 21 '13 at 01:23
  • 2
    @HighCore It allows an interface to both declare an optional method to the contract and define its behavior consistently when the implement or opts to omit it. A good example is `Iterator.remove` whose documentation stipulates that those not supporting removal of elements must throw a specific exception. Using this feature allows the interface to enforce this programmatically. – David Harkness Dec 22 '13 at 18:39
  • 1
    @DavidHarkness that sounds like what `abstract classes` are for. It doesn't explain why java has taken this horrible path of breaking OOP even worse by introducing executable code in interfaces. C# resolves that in a beautiful way by introducing `Extension Methods`, whereas java seems to be doing a big effort to induce developers to produce even shittier code. – Federico Berasategui Dec 23 '13 at 03:15
  • 1
    HighCore This provides one of the benefits of multiple inheritance that I miss from C++ and Python. I really like that each language solves similar problems differently. To each his own I guess. – David Harkness Dec 23 '13 at 05:05
  • 2
    Default methods cannot access any internal state of the object, so they do not hurt encapsulation. They are essentially the same as a static method except they can be overridden. I'm failing to see what is "broken" by this. – MikeFHay Dec 23 '13 at 11:26
  • 2
    The [holy scriptures](http://en.wikipedia.org/wiki/Cargo_cult_programming) clearly state that only heretics implement methods in interfaces. – Idan Arye Dec 23 '13 at 16:12
1

But if the intention is for every class that implements Matcher to also extend BaseMatcher, why use an interface at all? Why not just make Matcher an abstract class in the first place and have all other matchers extend it?

By separating interface and implementation (abstract class is still an implementation) you comply with Dependency Inversion Principle. Do not confuse with dependency injection, nothing in common. You might notice that, in Hamcrest interface is kept in hamcrest-api package, while abstract class is in hamcrest-core. This provides low coupling, because implementation depends only on interfaces but not on other implementation. A good book on this topic is: Interface Oriented Design: With Patterns.

Is there some advantage to doing it the way Hamcrest has done it? Or is this a great example of bad practice?

The solution in this example looks ugly. I think comment is enough. Making such stub methods is redundant. I wouldn't follow this approach.

Mikhail
  • 4,175
  • 15
  • 31
  • I agree and understand the Dependency Inversion Principle. What gets me is that, in spite of *technically* following the Dependency Inversion Principle and low coupling, Hamcrest insists that they don't like it by forcing you to implement a useless method. I still think that binary compatibility is the most likely explanation, but +1 for mentioning Dependency Inversion Principle. – James Dunn Dec 24 '13 at 15:10
  • I think this is an example of bad practice. Saying honestly, I dont understand your idea about binary compatibility. – Mikhail Dec 25 '13 at 06:20