Code-Free Constructor Violations

[StuporHero]

ListOf, MapOf, and SetOf all contain some variation of a loop that assigns the values of the src argument to the wrapped Collection in their constructors. As I understand it, this is a violation of the Code-Free Constructor Principle. The fix is fairly straightforward and shouldn't affect users of these classes. The constructors for CollectionEnvelope and IterableEnvelope should be made to accept Scalar<Collection<T>> and Scalar<Iterable<T>> respectively, and the action of adding the elements should be performed in a Lambda wrapped in a Sticky<Collection<T>>.

/**
 * Ctor.
 * @param src An {@link Iterable}
 */
public ListOf(final Iterable<T> src) {
    super(
        new Sticky<>(
            () -> {
                final Collection<T> collection = new LinkedList<>();
                src.forEach(collection::add);
                return collection;
            }
        )
    );        
}

[victornoel]

@StuporHero even though the intention is good, we won't support this for two reasons:

• envelopes should only be envelopes and not have to deal with scalars
• code free constructor makes sense for real behavioural objects, but ListOf, MapOf and SetOf are not behavioural objects, they are datastructures and thus we can either completely remove them from cactoos and just keep them as is in order to ease interoperability with the java collection framework.

Not also that in practice, using a scalar or a code free constructor won't change the overall design of those classes, so it's not worth the effort to do so just for the sake of doing it.

That being said, let's not close this ticket right away, maybe some nice idea will come later on how to handle this situation in a nicer way! Thanks

[StuporHero]

@victornoel I don't understand the point about envelopes not dealing with Scalars. Can you elaborate the reasoning behind it? Actually, I don't understand the other point, either.

[victornoel]

@StuporHero envelopes takes an instance of the interface they implement. If they start taking scalars, then they will have to call value() in their constructors: then you will not have code free constructors in envelopes!

[StuporHero]

@victornoel You wouldn't have to call value() in the constructor. You'd call it in the methods where they delegate to the encapsulated class.

[StuporHero]

@victornoel Also, it is conceivable that an Iterable performs real work in order to generate the elements it is iterating (for example, an Iterable that instantiates objects from a web service or database). This work would be triggered in the constructor of these classes as they are now.

[victornoel]

@StuporHero please see #1384, #947, #1287 and #1335 about envelopes and #1008 about collections envelope in particular.

If you use scalar in them, you will make subclasses of those envelopes very awkward to use, so it was decided to go the way described in those issues.

[victornoel]

@StuporHero concerning iterable, you can use IterableOf if your want to define an actual behaviour to generate values. Iterable is not a data structures interface but a real object oriented one, that's why we have a bunch of decorators for it.

[StuporHero]

@victornoel You're conflating the use of Scalar with the actual problems in the issues you linked. Typically the problem was that the envelope classes implemented common behavior above and beyond simply delegating method calls to the underlying implementations. I agree that this is bad. Using a Scalar does not do this.

The only issue that looks like Scalar may have been the problem is #942, but if you examine it closely, it is clear the issue was that it used a Scalar<String> instead of a Scalar<Text>. Look at the offending line. Let's step through what's happening.

1. this.origin, which is a Scalar<String>, is wrapped in an UncheckedScalar<String>. String is inferred by the diamond operator.
2. value() is called on the UncheckedScalar<String> which evaluates this.origin and either returns a String or throws an UncheckedIOException wrapping an IOException wrapping the underlying Exception. Let's assume the String value is returned.
3. equals() is called on the returned String value and is given the Object passed into the currently executing equals() method. In the test that was failing, the argument supplied to this method was of type Text. We just called equals() on a String and compared it to a Text which can be expected to fail. This could have been fixed by simply calling toString() on obj, but replacing the Scalar<String> with a Scalar<Text> would have made more sense. The Scalar, itself, had nothing to do with it.

We can safely reintroduce Scalar to the envelope classes as long as the type parameter is the same as the interface we intend to envelope. We can do this by introducing a constructor that accepts the Scalar and modifying the current constructor to wrap the incoming value in a Scalar to avoid breaking any current subclasses outside of the ones I'm proposing to change. I'm NOT proposing we reintroduce any additional logic.

[victornoel]

@StuporHero I think maybe you read them too fast, because there are two concerns with the old envelopes and they were well explained in those issues. I know because I wrote them, I designed and organized their applying in cactoos and also implemented some of those issues myself.

There was the extra functionality, but there was also the laziness of using scalar.
Both are unwanted for envelopes, I recommend you read about envelopes in yegor's blog maybe.

If we take for example the Set interface (it is the same for all concrete collections, list, map, set, etc, but not iterable or iterator!):

• the Set interface is about a mutable data structures of type set
• if you use scalar in the envelope, it means every call to every method will recreate the Set instead of manipulation the same state.

If you desire to have this behaviour, it is possible to make a new class LazySet that implements Set. But envelopes are NOT about that at all.

So let's say you have a requirement that is not answered by existing abstractions in cactoos: please describe it here and we can find a solution for it. But we won't reintroduce scalar in envelopes for sure.

[StuporHero]

@victornoel

the action of adding the elements should be performed in a Lambda wrapped in a Sticky<Collection<T>>

My code example also showed as much. This is not a real objection because I already provided this solution.

The only way you could gather that I read through what you linked too quickly is that you are ideologically opposed to my suggestion and uninterested in critically examining your own position. I spent 2 hours not just reading the issues you linked but diving into the code to understand what the problems were and why the solutions fixed them. I also reread @yegor256's blog posts concerning envelopes to understand his motivation for coming up with the concept because I like to understand why I'm wrong and because you have so far failed to impart that understanding at either the philosophical or practical level. This was all to ensure that I provided a carefully considered and thoughtful response to which I received this lazy slop. Give me a real, solid reason why Scalar should not be used in an envelope because none of the source material suggests there is one.

[StuporHero]

@victornoel Here is direct evidence that you are patently wrong about not being able to use Scalar in an envelope.

This is the only way. This is how it should be.

[victornoel]

@StuporHero before answering about cactoos, let me say something: we started on the wrong foot and it seems that the more you talk the more uncivil you are being.
I'm not at your service and I don't owe you anything. I'm doing ARC here out of my free time and I have thought a lot about the reasons those decisions were taken. I can be mistaken and ready to accept it if you don't attack people ad nominem and be aggressive like that. Please be polite and open to having a discussion where people don't agree with you or I will just ignore you.

That being said, I'm still convinced the current design is the right decision, I will answer a bit later when I have time and I'm not being angry at the way this conversation went.

[victornoel]

@StuporHero so first: I disagree with @yegor256 in his video. The flexibility he talks about can be achieved in a much more simpler way than by having envelopes wrapping scalar and I am going to show you how, I hope you will see that my proposal (which is how it is implemented in cactoos) is equivalent but more simple and elegant.

According to all this discussion and this video, I understand that the objective is to get the flexibility to choose WHEN the delegated object is created, and the argument is that by using scalar in the envelope, it allows to either choose a bare scalar or a sticky one (the "corner case" yegor talks about).
If you don't agree with this, just say so, please don't read the rest of this comment.

I believe we can achieve the same like this:

public interface SomeInterface {

    void doA();

    String someB();

    public abstract class SomeInterfaceEnvelope implements SomeInterface {

        private final SomeInterface wrapped;

        public SomeInterfaceEnvelope(SomeInterface wrapped) {
            this.wrapped = wrapped;
        }

        public void doA() {
            wrapped.doA();
        }

        public String someB() {
            return wrapped.someB();
        }
    }

    public final class SomeImpl implements SomeInterface {

        private final String txt;

        public SomeImpl(String txt) {
            this.txt = txt;
        }

        @Override
        public void doA() {
            System.out.println(txt);
        }

        @Override
        public String someB() {
            return txt.toUpperCase();
        }
    }

    public final class IntImpl extends SomeInterfaceEnvelope {
        public IntImpl(int value) {
            super(new SomeImpl("" + value));
        }
    }

    public final class LazySomeInterface implements SomeInterface {

        private final Scalar<SomeInterface> wrapped;

        public LazySomeInterface(Scalar<SomeInterface> wrapped) {
            this.wrapped = wrapped;
        }

        @Override
        public void doA() {
            wrapped.value().doA();
        }

        @Override
        public String someB() {
            return wrapped.value().someB();
        }
    }

    public final class RandomImpl extends SomeInterfaceEnvelope {
        public RandomImpl() {
            this(new Random());
        }

        public RandomImpl(final Random random) {
            super(new LazySomeInterface(() -> new IntImpl(random.nextInt())));
        }
    }
}

Here is what it allows:

• The simpler use of this is SomeImple and IntImpl: they directly translate as implementation the interface. They implement SomeInterface behaviour in a straightforward way without any surprise.
• If you DO need to get some control over when things happen, using scalar for example, you can use LazySomeInterface and use it as in RandomImpl.

Here is why it is better:

• there are no surprises, if you do need some special behaviour (and I believe in Java, lazyness is a special behaviour), then you explicitly do so. You don't have implicitness.
• the responsibilities are better distributed: each class do exactly one thing, not many
• it is easier to read and maintain
• envelopes can very easily be used for building decorators without problem

Concerning the specific case of Iterable, List and other classes in the collection package:

• Iterable IS NOT a collection interface, it is a typical object interface: it has few methods which provide behaviour.
• List and all subclasses of Collection ARE datastructure interface: they store data and allow to manipulate it
• in cactoos Iterable are the principal mean of providing decorators to manipulate stuffs that can be iterated over
• List and other collections are just there for interoperability with the java lib
• If you need a way to create a list using a scalar that is called for every method, we can just introduce a new LazyList class.
  • To be honest, I still haven't seen an actual example of what you really want to do, so I'm hoping I'm correct about your requirements here…

All of this stems from the following that is very important IMHO: most correctly designed object-oriented classes are already lazy by nature

• they store some state, i.e., other objects and their methods are responsible of implementing behaviour based on the state (i.e., they don't return just data)
• if you follow EO, most of your classes will be like that OR they will be decorators over those classes
• envelopes are an easy way to build decorators or your own implementation only comprised of other implementation of the same interface (as in the example above)

[victornoel]

@yegor256 as I said in telegram, it would be good to have your opinion about this architectural choice as your are the PO of Cactoos. I believe the previous comment is enough to cover the whole rationale of this choice.

[StuporHero]

@victornoel Your reading comprehension skills are remarkably poor. The crux of this problem is that these classes do not follow EO guidelines in the library that claims to be built around EO. It's in the title of this ticket. You're attempting to argue that their current design is somehow preferable except that they now poison every constructor they are used in with code. There is no technical reason preventing the proper implementation, either. You told me to refer to @yegor256's blog to understand why you are right, and when I showed you the video attached to that blog making exactly the same argument I made to you, you said you disagreed with it. You can hold whatever opinions you like, but if I can't count on the EO library to comply with EO guidelines, there is no point in using it. I'd be better off forking it and maintaining a proper EO version for myself than wasting time talking to an ARC who is derelict in his duty to maintain this project according to its guiding principles.

[fabriciofx]

@StuporHero I'm sorry, but you crossed right now the line of healthy discussion. If you can't talk without offend other participants you're not welcome here.

[StuporHero]

@fabriciofx Do you have an opinion on the issue being discussed?

[fabriciofx]

@StuporHero I read all arguments and I agree with @victornoel. You're free to disagree, we can be (Victor and I) both wrong, but ALWAYS be POLITE in a discussion, not matters what happens.

[StuporHero]

@fabriciofx This all stemmed from @victornoel suggesting I read through things too quickly when I literally spent hours reading through the several links he sent as well as examining the source code and reading blog posts to try to understand why Scalar is bad here. Maybe that doesn't come across as rude and disrespectful to you two, but English is my first language, and that is not a way to talk to someone where I come from.

[StuporHero]

@victornoel I just looked at your code sample. If you want to keep the envelopes pure, that's fine, but understand that you need to use a lazy implementation in 100% of the constructors of classes that would otherwise require code in them in order for them not to violate the expectation established by the second principle linked in the README of this repository that all constructors in this library are code-free. Keeping the envelopes pure means you cannot use them for ListOf, etc. which would rob the envelopes of their purpose for virtually no benefit. If you have a different set of principles, they don't override the ones established as expectations with your users regardless of your status as ARC. If keeping constructors code-free is no longer a principle for this library, then it needs to be purged from the principles list or the link needs to be removed from the README along with the EO badge so your users don't use this library believing it conforms to them when it does not. The ARC of a library must uphold the expectations set with the library's users and not argue with them when they ask that the library be made to conform to the those expectations. You may decide how to implement the fix, but the principle supersedes both of us.

[fabriciofx]

@StuporHero nothing justify someone to be disrespectful. If @victornoel was disrespectful with you, just say it and wait. I'm sure he will apologize. But you don't need to do in the same way, right?

[StuporHero]

@fabriciofx I'm ready to get back to the topic of this ticket.

[victornoel]

@fabriciofx thank you for your mediation

@StuporHero I'm sorry I offended you, English is indeed not my first language but anyway if I could have avoided it, I would have. That being said, you have been really rude repeatedly and again after I asked you to stop. Please do stop.

Back to cactoos: let's not confuse the problem you raised and the solution you pushed for. I continue to think the current design in cactoos for envelope is the right one, and I already explained why.

Concerning code free constructors, for now I consider them a necessary evil simply because I don't know how to avoid it except by deleting those classes.
The only thing I can see is using lazy versions of those interfaces as explained in my code sample above but it won't be the same behaviour then.

If you can propose a solution, please do so, but again let's stop confusing stuffs and please take the time to write organized and non-aggressive comments.

[victornoel]

After some thoughts and some discussion in telegram about the purpose of Scalar, I think the only viable solution to the constructor free violation is to replace those classes, for example ListOf, by a Scalar<List> and have the implementation of value responsible of creating the list. E.g.:

public static final ListOf<T> implements Scalar<List<T>> {
  public ListOf(Iterable<T> iterable) {
    this.iterable = iterable;
  }

  public List<T> value() {
    List<T> list = new LinkedList<>();
    this.iterable.forEach(list::add);
    return list;
  } 
}

This means you cannot use it like it is currently used via new ListOf(xxx), but you would need to call new ListOf(xxx).value() on it to get the actual value. I don't think it is a problem though.

We would do the same for all the other implementation like this.

WDYT @fabriciofx @StuporHero?

[StuporHero]

@victornoel Stop saying things like, "let's not confuse the problem you raised and the solution you pushed for." THAT is rude and demonstrates that you don't understand what I'm asking for. I submitted an issue (there is code in some constructors), and I offered a possible solution (we could use Scalar in the envelopes). The solution is debatable, but rather than offer an alternative solution to my issue, you flatly refused to fix the issue because you didn't like my proposed solution showing that you confused my proposed solution with the issue, so, please, stick to the ISSUE and say nothing outside of it if you don't want me to brow beat you again.

Your currently proposed solution does not work because it would break a lot of code across many different libraries of my own. Why don't you just introduce the LazyListOf, etc. and use those in your constructor calls to the envelopes? It would look awkward because you'd either be wrapping the list twice for no reason or you'd skip using the envelope altogether which would call into question the purpose for its very existence, but thanks to your dogmatic position on the subject and your willingness to violate this project's principles to maintain that, it is the only solution that will satisfy us both. Also, having a discussion about it in Telegram is a violation of keeping everything in GitHub for posterity, so if there is any way you could somehow incorporate that discussion into this ticket, it would be appreciated.