MatcherEnvelope and TextMatcherEnvelope are just inheritance hidden behind EO names

[victornoel]

Problem

I believe the two TextMatcherEnvelope and MatcherEnvelope used in cactoos-matchers are a hijacking of the term envelope just to justify using inheritance in a bad way.
Now we have many objects whose behaviour is distributed in their own and parent class.

Envelopes

An envelope is an abstract class meant to help decorate object of a certain type, let's call it T, in order to ease applying the decorator pattern in EO around objects of type T.

Usually an envelope only exists to delegate calls to a wrapped object of type T according to the contract (the methods) of T.

From there there should be some base implementations of T that implemented some specific behaviour, and decorators of T can just extend the envelope and use the base implementations to pass it to super.

Here is an example of a proper envelope: https://github.com/yegor256/cactoos/blob/master/src/main/java/org/cactoos/func/FuncEnvelope.java

More details here: http://www.yegor256.com/2017/01/31/decorating-envelopes.html

Note: sometimes in the EO world, envelopes are called wrap (XXXWrap).

Solution

In cactoos-matchers, there should be only a MatcherEnvelope that simply delegates the methods of Matcher without imposing any specific behaviour and then:

• we add TextMatcher (not extending MatcherEnvelope) that would contain the actual behaviour currently implemented in TextMatcherEnvelope. To use it, TextIs for example, would extend MatcherEnvelope and rely on TextMatcher to pass to super.
• MatcherOf should take the three currently constructor arguments that MatcherEnvelope currently propose and implement the methods of Matcher with them.

    public MatcherOf(
        final Func<T, Boolean> match,
        final Proc<Description> description,
        final BiProc<T, Description> mismatch
    ) {

Note on Matcher library

Of course because we rely on Matcher, an abstraction introduced by a non EO library, some of our matchers (all those that are not defined in terms of other view decoration) will still need to extends TypeSafeDiagnosingMatcher.

[0crat]

@llorllale/z please, pay attention to this issue

[victornoel]

@llorllale in a way this is similar to yegor256/cactoos#947 and the solution that was implemented there.

[llorllale]

@victornoel

using inheritance in a bad way

Please elaborate.

Now we have many objects whose behaviour is distributed in their own and parent class.

If this is the reason then the counter is that the interface contracts specify (or should) specify behavior applicable across all implementations. Eg. "all Text implementations are equal so long as their string content is equal".

You might argue: we can implement a "ComparableText" or something similar. Someone might counter with a usability argument. I'm not sure.

[victornoel]

@llorllale

using inheritance in a bad way
Please elaborate.

Sorry, it is badly phrased, I should have said using inheritance which is generally bad. I think that most of the arguments are covered for example here: https://www.yegor256.com/2016/09/13/inheritance-is-procedural.html.

But let me clear: I don't say you shouldn't be using inheritance, I am saying that if you do, please don't call the parent class XXXEnvelope because that's not what an envelope is (see yegor256/cactoos#1154 (comment) for more details on that). Just call it AbstractCommonMatcher or AbstractCommonText for the example you gave above :)

If this is the reason then the counter is that the interface contracts specify (or should) specify behavior applicable across all implementations. Eg. "all Text implementations are equal so long as their string content is equal".

The problem with this is that the ONLY way to reuse that common behaviour, is to inherit from AbstractCommonText, while if you provide an default implementation of Text based on a Scalar that returns a String, let's call it ScalarText, you don't impose that people use inheritance. They can create an instance of ScalarText and delegate themselves, they can use the envelope to take care of delegation, they can wrap this instance inside another decorator (which you cannot do with your AbstractCommonText by the way).

I didn't think about this example before, but with inheritance, you loose the capacity to compose multiple decorator together also.

You might argue: we can implement a "ComparableText" or something similar. Someone might counter with a usability argument. I'm not sure.

The usability argument is moot to me, see again the discussion in yegor256/cactoos#1154 (comment) for that, I don't think it is really less usable in terms of typing stuffs, and the advantages are greater if you use composition over inheritance.

[victornoel]

@llorllale arg, I again forgot to ping you in the above comment, sorry, not my day today :) it's fixed

[llorllale]

@victornoel

I don't say you shouldn't be using inheritance, I am saying that if you do, please don't call the parent class XXXEnvelope because that's not what an envelope is

I posed the question to yegor on this blog: http://disq.us/p/234j0ov

Here's how I see it:

• All Texts are equal if their contents are equal.
• If the above does not satisfy your requirements then you shouldn't be modelling your object as a Text. A composite is probably more fitting.
• Envelopes ease the implementation of interfaces. equals() is part of the Text interface as dozens of unit tests show.
• TextEnvelope could delegate to the enveloped Text at the risk of the developer not using the right decorator that implements equals. But then again, if he doesn't he breaks the Text contract and LSP.
• Following from previous point: in order to properly implement Text you'd need to use that decorator everywhere - usability

[victornoel]

@llorllale I'm sorry but I think you are missing the point. I have been successfully be using envelopes as I describe them and everything you just said in that comment is not related to the question of envelopes.

All Texts are equal if their contents are equal.

Yes: however a Text is implemented, its equals method should be respecting the contract, we agree.

If the above does not satisfy your requirements then you shouldn't be modelling your object as a Text. A composite is probably more fitting.

Yes, but it's tangential to the discussion here, I don't see why you bring that up.

Envelopes ease the implementation of interfaces. equals() is part of the Text interface as dozens of unit tests show.

I agree with you that it is, I never said the opposite, I don't understand why you try to convince me of something I already agree with.

I would add the following: if equals is part of the contract of Text (and it is), then in TextEnvelope equals should just be delegated to the wrapped object. A TextEnvelope wraps a Text object, so if a class extends TextEnvelope, it means:

• that it implements the Text interface and its contract, equals included
• it delegate to the wrapped object the behaviour of Text, equals included

TextEnvelope could delegate to the enveloped Text at the risk of the developer not using the right decorator that implements equals. But then again, if he doesn't he breaks the Text contract and LSP.

Yes, we agree: the only way to extend TextEnvelope is to pass to its constructor a Text object that should respect the contract, and thus equals contract, to if TextEnvelope delegates equals, it will respect LSP because the wrapped object does. I'm not clear what is your actual point here?

Following from previous point: in order to properly implement Text you'd need to use that decorator everywhere - usability

No, it's incorrect, if you directly implement Text, you can, but you just have the responsibility of implementing equals, because it is part of its contract. For example, for some implementations of Text, we could get a more efficient implementation of equals than comparing the whole String who knows. If one desire, one can implement it, there is no obligation to reuse the library provided implementation of equals.

Finally:

I posed the question to yegor on this blog: http://disq.us/p/234j0ov

You posed a good question, but it's a vain question because our TextEnvelope is not even similar to what is discussed in Yegor's blog post! Our TextEnvelope doesn't wrap a Text, that's a first sign that something is amiss: just check the RsWrap example, do you see it taking a head and a body in the primary constructor (like TextEnvelope takes a Scalar<String>)? No, it takes a Response and delegates its methods.

I'm just asking for those separate things, and maybe you can decide to only apply some, I don't know:

1. don't name this abstract class TextEnvelope because it's not an envelope. Naming matter. Let's call it AbstractText or something like that.
2. do introduce a real TextEnvelope that takes a Text and delegates all the Text methods, including equals, hashCode and toString.
3. Rename AbstractText into ScalarText and make it final and non-abstract. It is already self-sufficient. and simply have all the Text implementation that extended it to extend TextEnvelope, delegating to ScalarText by passing it to the super constructor. Or by passing another more relevant implementation of Text when needed.

[victornoel]

@paulodamaso for the record, in cactoos things went forward with having pure envelopes and common features in the real implementation instead of abstract classes, see yegor256/cactoos#947

[victornoel]

@0crat in

[0crat]

The job #135 assigned to @marceloamadeu/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

[marceloamadeu]

@0crat refuse

[0crat]

@0crat refuse (here)

@marceloamadeu The user @marceloamadeu/z resigned from #135, please stop working. Reason for job resignation: Order was cancelled

[0crat]

@0crat refuse (here)

@marceloamadeu Job refused in 0 hours - no penalty, see §6

[0crat]

The job #135 assigned to @longstone/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

[0crat]

The architect of the project has changed; @paulodamaso/z is not at this role anymore; @victornoel/z is the architect now

[victornoel]

@0crat in

[victornoel]

@0crat assign @longstone

[0crat]

@0crat assign @longstone (here)

@victornoel The job #135 assigned to @longstone/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

[0crat]

@0crat assign @longstone (here)

@victornoel There is an unrecoverable failure on my side. Please, submit it here:

PID: 4@d30d334c-b8f5-4e54-8f5c-30e1e363cf4f, thread: PQ-C63314D6Z
com.jcabi.xml.StrictXML[124] java.lang.IllegalArgumentException: 1 error(s) in XML document: -1:-1: Duplicate unique value [gh:llorllale/cactoos-matchers#135] declared for identity constraint of element "agenda".

0.53.15: CID: 517db924-424d-4421-9211-a99c20b75a3b, Type: "Order was given"

[victornoel]

@0crat assign me

[victornoel]

@0crat wait for #163

[0pdd]

@victornoel the puzzle #165 is still not solved.

[victornoel]

@0crat wait for #195.

[0crat]

@0crat wait for #195. (here)

@victornoel Job #135 is already on hold

[0pdd]

@victornoel 2 puzzles #224, #225 are still not solved; solved: #165.

[0pdd]

@victornoel the puzzle #224 is still not solved; solved: #165, #225.