Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#947) IterableEnvelope only delegates and IterableOf has behaviour #1154

Merged

Conversation

victornoel
Copy link
Collaborator

This is for #947.

I started with IterableEnvelope and added todos for ListEnvelope and CollectionEnvelope.

I had to:

  • move some of the methods from the envelope to IterableOf. I believe it makes sense for this to be.
  • use IterableOf when extending IterableEnvelope to get the same behaviour.
  • IterableOf now publicly takes a Scalar<Iterator> since it is the root of its behaviour
  • I had to add a Scalar named Mapped to simplify implementation in some places

It's a bit big, but I feel like it had to happen because of the high number of classes extending IterableEnvelope.

@0crat
Copy link
Collaborator

0crat commented Jun 30, 2019

Job #1154 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Jun 30, 2019

This pull request #1154 is assigned to @vzurauskas/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

@victornoel
Copy link
Collaborator Author

@vzurauskas ping?

@victornoel victornoel force-pushed the 947-iterable-envelope-is-just-an-envelope branch from e754c90 to 2dc9623 Compare July 6, 2019 16:55
@codecov-io
Copy link

codecov-io commented Jul 6, 2019

Codecov Report

Merging #1154 into master will decrease coverage by 0.02%.
The diff coverage is 94.38%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1154      +/-   ##
============================================
- Coverage     89.35%   89.33%   -0.03%     
- Complexity     1588     1599      +11     
============================================
  Files           271      272       +1     
  Lines          3888     3899      +11     
  Branches        215      214       -1     
============================================
+ Hits           3474     3483       +9     
- Misses          379      381       +2     
  Partials         35       35
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/list/ListEnvelope.java 100% <ø> (ø) 12 <0> (ø) ⬇️
...ava/org/cactoos/collection/CollectionEnvelope.java 100% <ø> (ø) 28 <0> (ø) ⬇️
...ain/java/org/cactoos/iterable/IterableOfChars.java 100% <100%> (ø) 1 <1> (ø) ⬇️
src/main/java/org/cactoos/iterable/Reversed.java 66.66% <100%> (ø) 2 <1> (ø) ⬇️
...in/java/org/cactoos/iterable/IterableOfShorts.java 100% <100%> (ø) 1 <1> (ø) ⬇️
...ain/java/org/cactoos/iterable/IterableOfLongs.java 100% <100%> (ø) 1 <1> (ø) ⬇️
src/main/java/org/cactoos/iterable/HeadOf.java 66.66% <100%> (+6.66%) 2 <2> (+1) ⬆️
src/main/java/org/cactoos/iterable/Mapped.java 100% <100%> (ø) 3 <2> (ø) ⬇️
...n/java/org/cactoos/iterable/IterableOfDoubles.java 100% <100%> (ø) 1 <1> (ø) ⬇️
...main/java/org/cactoos/iterable/IterableOfInts.java 100% <100%> (ø) 1 <1> (ø) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ff9e6d...b092754. Read the comment docs.

@vzurauskas
Copy link
Contributor

@victornoel Sorry, still fighting the ton of REV jobs in my agenda. A new option to limit them should be introduced soon, it should be better then.

@vzurauskas
Copy link
Contributor

@0crat refuse

@0crat
Copy link
Collaborator

0crat commented Jul 8, 2019

@0crat refuse (here)

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

@0crat
Copy link
Collaborator

0crat commented Jul 8, 2019

Tasks refusal is discouraged, see §6: -15 point(s) just awarded to @vzurauskas/z

@0crat
Copy link
Collaborator

0crat commented Jul 10, 2019

This pull request #1154 is assigned to @scristalli/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

Copy link
Contributor

@scristalli scristalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel I left a few comments

src/main/java/org/cactoos/scalar/Mapped.java Show resolved Hide resolved
@@ -49,13 +47,7 @@ public Solid(final X... src) {
* @param iterable The iterable
*/
public Solid(final Iterable<X> iterable) {
super(
new NoNulls<>(
new org.cactoos.scalar.Solid<>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel why remove NoNulls and Solid here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scristalli because it was useless, the tests passes without them: I think the NoNulls is a remnant of the past and that the Solid was only needed because IterableEnvelope was taking a Scalar before.

new org.cactoos.scalar.Sticky<Iterable<X>>(
() -> {
final Collection<X> temp = new LinkedList<>();
for (final X item : iterable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel is it possible to refactor this part and use an object-oriented approach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scristalli I believe it's out of scope of the issue solved, you can open an issue in cactoos if you want. I tried to think about it, but it's not clear what is best between using ListOf + scalar.Sticky, or directly list.Sticky, etc. You can add this information in the issue you create for example.

*/
public final class IterableOf<X> extends IterableEnvelope<X> {
@SuppressWarnings("PMD.OnlyOneConstructorShouldDoInitialization")
public final class IterableOf<X> implements Iterable<X> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel what's the reason for implementing Iterable now instead of keep extending IterableEnvelope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scristalli it was kind of explained in the description: the envelope does not contain any specific behaviour that is implemented by IterableOf. IterableOf is not a decorator on Iterable, it's an implementation of Iterable. I could have extended IterableEnvelope, but I believe it would have been a design mistake semantically. Also, now that I think about it, it wouldn't have been possible to implement toString, equals and hashCode if I was extending IterableEnvelope.

@victornoel victornoel force-pushed the 947-iterable-envelope-is-just-an-envelope branch from 2dc9623 to e71c2ba Compare July 13, 2019 09:53
@victornoel
Copy link
Collaborator Author

@scristalli I've pushed some change and answere your comments, thanks

@scristalli
Copy link
Contributor

@llorllale I approve the changes

* <p>There is no thread-safety guarantee.
*
* <p>This class implements {@link Scalar}, which throws a checked
* {@link IOException}. This may not be convenient in many cases. To make
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel it's just an Exception now

@llorllale
Copy link
Contributor

@victornoel I come here from llorllale/cactoos-matchers#135

I'm not sure about the consequences of transferring equals, toString and hashcode from the envelopes to a "special" implementation of the interface.

From a usability perspective, it's the difference between

Existing way:

// we get equals, hashcode and toString for free
public final MyIterable<T> extends IterableEnvelope<T>{
    public MyIterable(final T ...elements) {
        super(args);
   }
}

And your way:

// we need IterableOf to get equals, hashcode and toString
public final MyIterable<T> extends IterableEnvelope<T>{
    public MyIterable(final T ...elements) {
        super(new IterableOf<>(args));
   }
}

Also, it's not clear why IterableOf should be the "special" class that provides those methods. They probably belong in another class? In which case, your way would involve additional decoration.

@victornoel
Copy link
Collaborator Author

victornoel commented Jul 17, 2019

@llorllale First, let me remind you that there is a discussion in #947 that arrived to the conclusion that envelope should only delegates to wrapped object. Hence this PR. But I completely share the design decisions here, so let me try to explain them.

To me: one class = one responsibility.

An envelope, by its name, is something whose responsibility is to envelope. Actually I see it as a pattern, it doesn't implement behaviour, it helps implementing behaviour in other class by easing the delegation of implementation to another class. As in your second example. The problem with it, is that it's useless as an example, you could have used directly IterableOf itself. Now if you look at say, Cycled, in this PR: https://github.com/yegor256/cactoos/pull/1154/files#diff-7aac25700ee98a7068c93995c10fa5bc, it makes sense: you want to reuse some implementation, but you want to use it with specific constructor parameters.

Also, it's not clear why IterableOf should be the "special" class that provides those methods. They probably belong in another class? In which case, your way would involve additional decoration.

You are right, it doesn't. I could introduce a class named DefaultIterable, BasicIterable, CommonIterable or maybe it would be better to call it ScalarIterable, whose purpose is to implement all the contract of Iterable, based on a Scalar that returns a Iterator.

Then IterableOf would extend IterableEnvelope and delegate to ScalarIterable in order to take advantage of its behaviour's implementation.

We actually could also implement ArrayIterable which is an iterable based on an array, so it would simplify implementation and performance of the methods (behaviour). Then when other classes would need to delegate to one of the classes implementing the common behaviour, they can choose: do I delegate to ArrayIterable or do I delegate to ScalarIterable? And so on.

See there already you have two different implementation of the same behaviour: IterableEnvelope is only there to help ease the usage of one or the other in your decorator, without imposing one on you as it is currently the case (and was the initial problem from #947, kind of)

@victornoel
Copy link
Collaborator Author

@llorllale oups, forgot to ping you in my previous answer, I edited it :)

@victornoel
Copy link
Collaborator Author

@llorllale you will notice I did a mixup in my head between Text and Iterable, I will edit it, sorry about that :D

@victornoel
Copy link
Collaborator Author

@llorllale it's done, sorry again for the confusion

@victornoel
Copy link
Collaborator Author

victornoel commented Jul 17, 2019

@llorllale two more points:

  1. At the very least, if you want to use inheritance to reuse behaviour, do not call the parent class an envelope, but call it AbstractIterable or AbstractCommonIterable.
  2. by having a ScalarIterable for example as above instead of an AbstractIterable, is that you can compose multiple decorator while you can't with inheritance.

@victornoel
Copy link
Collaborator Author

@llorllale can we get some conlusion on this and get it merged if it's ok :)

@llorllale
Copy link
Contributor

@victornoel I appreciate your insights but I'm afraid that discussion is not over yet. You decide - you can either modify this PR or close it for now.

@victornoel
Copy link
Collaborator Author

@llorllale ok, what would you want me to change then so that this can be merged?

@0crat
Copy link
Collaborator

0crat commented Aug 18, 2019

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@0crat
Copy link
Collaborator

0crat commented Aug 28, 2019

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@0crat
Copy link
Collaborator

0crat commented Sep 2, 2019

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@0crat
Copy link
Collaborator

0crat commented Sep 7, 2019

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@0crat
Copy link
Collaborator

0crat commented Sep 11, 2019

This pull request #1154 is assigned to @fabriciofx/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

Copy link
Contributor

@fabriciofx fabriciofx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel I agree with the new design. Can you fix the minor corrections.

*/
public IterableEnvelope(final Scalar<Iterable<X>> scalar) {
this.iterable = new Unchecked<>(scalar);
public IterableEnvelope(final Iterable<X> wrapped) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel The parameter's name and attribute must be different. Please, can you change it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciofx why should they be different? I'm not clear on the rational behind that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel it's a bad practice because is harder than read (different name is easier). Who said it? Yegor and if you pay attention, you'll see these names are (or should be) different ia cactoos' classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciofx thx, didn't know that, I will change them, the argument is convincing :) btw do you known if there is a blog post about that matter somewhere, I would be interested on reading about it more!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel I don't remember if there is a blog post about it, but once I pushed a PR and Yegor alert me about it...

*/
public Mapped(final Func<T, U> func, final Scalar<T> scalar) {
this.origin = scalar;
this.func = func;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel The parameter's name and attribute must be different. Please, can you change it?

final IterableOf<Integer> iterable = new IterableOf<>(1, 2);
new Assertion<>(
"Must have equal hashCode for Iterables with equal content",
iterable.hashCode(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel Move new IterableOf<>(1, 2) to this line.

final IterableOf<Integer> iterable = new IterableOf<>(1, 2);
new Assertion<>(
"Must be equal to itself",
iterable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel Move new IterableOf<>(1, 2) to this line.

final IterableOf<Integer> iterable = new IterableOf<>(1, 2);
new Assertion<>(
"Must be equal to Iterable with the same elements",
iterable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel Move new IterableOf<>(1, 2) to this line.

final IterableOf<Integer> iterable = new IterableOf<>();
new Assertion<>(
"Empty Iterable must be equal to empty Iterable",
iterable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel Move new IterableOf<>() to this line.

@victornoel victornoel force-pushed the 947-iterable-envelope-is-just-an-envelope branch from e71c2ba to d89f644 Compare September 11, 2019 19:43
Copy link
Contributor

@fabriciofx fabriciofx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel please, check it out.

@fabriciofx
Copy link
Contributor

fabriciofx commented Sep 14, 2019

@paulodamaso please, check it out why travis and appveyour fail. I cant' approve a PR that fails build.

@victornoel victornoel force-pushed the 947-iterable-envelope-is-just-an-envelope branch 2 times, most recently from 5ff830b to 2a892cb Compare September 14, 2019 08:57
@victornoel
Copy link
Collaborator Author

@fabriciofx I've rebased on master for the build to pass and applied the change you asked for. It should be alright know

@victornoel victornoel force-pushed the 947-iterable-envelope-is-just-an-envelope branch from 2a892cb to 4325d95 Compare September 14, 2019 09:06
Copy link
Contributor

@fabriciofx fabriciofx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel just more few minor corrections and after that, it'll be free to merge.

@Test
public void notEqualsToObjectOfAnotherType() {
new Assertion<>(
"Must not equal to object of another type",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel please, change "Must.." for `"must.." and all strings below.

@Test
public void equalToEmptyIterable() {
new Assertion<>(
"Empty Iterable must be equal to empty Iterable",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel The same here: "empty...".

@victornoel victornoel force-pushed the 947-iterable-envelope-is-just-an-envelope branch 2 times, most recently from a505301 to 2379aa8 Compare September 15, 2019 09:14
@victornoel
Copy link
Collaborator Author

@fabriciofx it's done, thanks

@Test
public void transformsScalar() throws Exception {
new Assertion<>(
"Must transform scalar",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel you forgot here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciofx damned, sorry, now it's fixed

@victornoel victornoel force-pushed the 947-iterable-envelope-is-just-an-envelope branch from 2379aa8 to b092754 Compare September 15, 2019 15:05
@fabriciofx
Copy link
Contributor

@victornoel it seems fine to me. Congratulations for this nice PR! @paulodamaso can you merge it, please?

@paulodamaso
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Sep 15, 2019

@rultor merge

@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit b092754 into yegor256:master Sep 15, 2019
@rultor
Copy link
Collaborator

rultor commented Sep 15, 2019

@rultor merge

@paulodamaso Done! FYI, the full log is here (took me 15min)

@0crat 0crat removed the scope label Sep 15, 2019
@0crat
Copy link
Collaborator

0crat commented Sep 15, 2019

@paulodamaso/z all QAs are on vacation. Please handle that.

@0crat
Copy link
Collaborator

0crat commented Sep 15, 2019

The job #1154 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Sep 15, 2019

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @paulodamaso/z

@victornoel victornoel deleted the 947-iterable-envelope-is-just-an-envelope branch September 16, 2019 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants