cactoos: Code-Free Constructor Violations

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;
            }
        )
    );        
}

About this issue

  • Original URL
  • State: open
  • Created 4 years ago
  • Comments: 25 (15 by maintainers)

Most upvoted comments

@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 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)

@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

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

@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 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.

@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.