(#1184) CollectionEnvelope is just an envelope #1216

victornoel · 2019-11-02T18:26:25Z

This is for #1184

I had to work around all the strange behaviours that crept in the collection package for some time. There were many incoherence between javadoc, behaviour, different implementations of Collection, etc.

This is what I did:

as with IterableEnvelope, I moved out of the envelope everything that does not pertain to it and put it in CollectionOf so that it retains its behaviour.
I had to wrap most of the other implementations in Sticky because the previous behaviour of the envelope was to be sticky (if I don't, tests for Joined and Reversed stop working, because they were written so that those implementation were sticky and mutable).
I removed useless constructor taking Collection in most classes since Collection is an Iterable already, this should remove some extra overhead at runtime and simplify understanding of the code
I added a @todo to continue making things more coherent in this package

Honestly the collection package and CollectionOf in particular is very awkward because a Collection is an Iterable, it is not "based on an Iterable" like Iterable is "based on Iterator" but the javadoc and the whole abstraction seems to be designed as if it was the case. I almost wonder if we shouldn't just delete CollectionOf and leave List or Set be responsible of instantiating new collections. Even the decorators are strange to me. @paulodamaso WDYT?

0crat · 2019-11-02T18:27:13Z

Job #1216 is now in scope, role is REV

codecov-io · 2019-11-02T18:31:20Z

Codecov Report

Merging #1216 into master will decrease coverage by 0.04%.
The diff coverage is 95%.

@@             Coverage Diff              @@
##             master    #1216      +/-   ##
============================================
- Coverage     89.36%   89.32%   -0.05%     
- Complexity     1683     1686       +3     
============================================
  Files           280      280              
  Lines          4034     4028       -6     
  Branches        213      212       -1     
============================================
- Hits           3605     3598       -7     
- Misses          394      396       +2     
+ Partials         35       34       -1

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/cactoos/set/SetEnvelope.java	`0% <0%> (-100%)`	`0 <0> (-1)`
src/main/java/org/cactoos/collection/NoNulls.java	`63.15% <0%> (-5.42%)`	`13 <0> (ø)`
src/main/java/org/cactoos/scalar/HashCode.java	`100% <100%> (ø)`	`7 <2> (+2)`	⬆️
src/main/java/org/cactoos/collection/Filtered.java	`100% <100%> (ø)`	`2 <1> (ø)`	⬇️
...main/java/org/cactoos/collection/CollectionOf.java	`100% <100%> (ø)`	`29 <28> (+25)`	⬆️
src/main/java/org/cactoos/collection/HeadOf.java	`100% <100%> (ø)`	`2 <1> (-1)`	⬇️
src/main/java/org/cactoos/collection/Reversed.java	`75% <100%> (-5%)`	`2 <2> (-1)`
src/main/java/org/cactoos/collection/Shuffled.java	`100% <100%> (ø)`	`3 <2> (-1)`	⬇️
src/main/java/org/cactoos/collection/Synced.java	`100% <100%> (ø)`	`4 <1> (ø)`	⬇️
src/main/java/org/cactoos/collection/Sorted.java	`69.23% <100%> (+9.23%)`	`3 <2> (ø)`	⬇️
... and 11 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 9b3500c...95f5a04. Read the comment docs.

0crat · 2019-11-02T18:35:25Z

This pull request #1216 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

paulodamaso · 2019-11-06T03:03:46Z

@victornoel I liked your idea, but I'm not too sure about just removing CollectionOf. I think that if it is really useless it will become very obvious, so let's keep it for now and wait to that happen.

victornoel · 2019-11-06T17:27:16Z

@paulodamaso yes, I think you are right! also is there any way to remove @fabriciofx from the project? It is making us wait a lot of time everytime he is assigned...

fanifieiev · 2019-11-06T22:58:25Z

@paulodamaso yes, I think you are right! also is there any way to remove @fabriciofx from the project? It is making us wait a lot of time everytime he is assigned...

@paulodamaso , that's very true.

fabriciofx

@victornoel please, take a look.

src/main/java/org/cactoos/collection/CollectionOf.java

src/main/java/org/cactoos/collection/Sticky.java

src/main/java/org/cactoos/collection/Synced.java

fabriciofx · 2019-11-12T02:49:49Z

src/test/java/org/cactoos/collection/CollectionOfTest.java

+        iterator.next();
+        iterator.remove();
+        new Assertion<>(
+            "Iterator should be empty",


@victornoel please, change "Iterator should be empty" to "Must be Iterator empty".

@fabriciofx IMHO, in this case it does not sound properly in English. The correct form is:"Iterator must be empty" or "Must be empty"(if iterator is meant).

@fanifieiev is "Must be an empty Iterator" better?

@fabriciofx Well, yes, it is better, but still the meaning is not as MUST if you want to express obligation, but rather is a certainty.
You can have a look at the way 'must' is used at https://www.grammar.cl/english/must.htm or at https://www.crownacademyenglish.com/modal-verb-must-form-use-meaning/
So, what you suggest is called "Deductions and conclusions"

@fanifieiev so, what's your recommendation? Please, note (almost) all fail test messages in Cactoos starts with Must...

@fabriciofx "
As an alternative, "Must return an empty Iterator" is also good.

@fanifieiev I liked "Must return an empty Iterator". Thanks a lot @fanifieiev!!! ✌️ 😄

@fabriciofx You are welcome!

@fabriciofx @fanifieiev thx, I've applied this for all the other below too

@victornoel ok

src/test/java/org/cactoos/collection/CollectionOfTest.java

fabriciofx · 2019-11-12T02:52:30Z

src/test/java/org/cactoos/collection/CollectionOfTest.java

+        final Collection<String> envelope = new CollectionOf<>();
+        final Collection<String> collection = Collections.emptyList();
+        new Assertion<>(
+            "Empty envelope and collection should be equal",


@victornoel same above (with properly adaptation).

@fabriciofx actually I removed this one because the test is redundant with one of the above and it has nothing to do with envelope anymore

@victornoel ok

fabriciofx · 2019-11-12T02:52:36Z

src/test/java/org/cactoos/collection/CollectionOfTest.java

+        final Collection<String> collection = new ArrayList<>(1);
+        collection.add("a");
+        new Assertion<>(
+            "Envelope and collection should be equal",


@victornoel same above (with properly adaptation).

@fabriciofx actually I removed this one because the test is redundant with one of the above and it has nothing to do with envelope anymore

@victornoel ok

victornoel · 2019-11-16T13:19:56Z

@fabriciofx sorry for the delay, see the changes I made

fabriciofx · 2019-11-16T16:39:04Z

@victornoel please, fix first the problem with CIs.

victornoel · 2019-11-16T17:47:45Z

@fabriciofx it is fixed, thx for the heads up

fabriciofx

@victornoel please, take a look again.

fabriciofx · 2019-11-16T17:57:57Z

src/main/java/org/cactoos/collection/Mapped.java

-     */
-    public Mapped(final Func<X, Y> fnc, final Collection<X> src) {
-        super(() -> new CollectionOf<>(
+        super(new Sticky<>(


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

fabriciofx · 2019-11-16T17:58:05Z

src/main/java/org/cactoos/collection/Joined.java

    }

    /**
     * Ctor.
     * @param list Items to concatenate
     */
    public Joined(final Iterable<Iterable<X>> list) {
-        super(() -> new CollectionOf<X>(
+        super(new Sticky<>(


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

fabriciofx · 2019-11-16T17:58:12Z

src/main/java/org/cactoos/collection/HeadOf.java

-     */
-    public HeadOf(final int num, final Collection<T> src) {
-        super(() -> new CollectionOf<T>(
+        super(new Sticky<T>(


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

fabriciofx · 2019-11-16T17:58:21Z

src/main/java/org/cactoos/collection/Filtered.java

@@ -53,7 +53,7 @@ public Filtered(final Func<X, Boolean> func, final X... src) {
     * @param src Source collection
     */
    public Filtered(final Func<X, Boolean> func, final Iterable<X> src) {
-        super(() -> new CollectionOf<>(
+        super(new Sticky<>(


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

fabriciofx · 2019-11-16T17:58:37Z

src/main/java/org/cactoos/collection/CollectionOf.java

@@ -60,13 +72,131 @@ public CollectionOf(final T... array) {
     * @param src An {@link Iterable}
     */
    public CollectionOf(final Iterable<T> src) {
-        super(() -> {
+        this(() -> {


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

@fabriciofx I don't think it is going to look better for a lambda like this...

fabriciofx · 2019-11-16T18:01:29Z

src/main/java/org/cactoos/collection/CollectionOf.java

+    // @checkstyle MagicNumberCheck (30 lines)
+    @Override
+    public int hashCode() {
+        return new Unchecked<>(


@victornoel please, use HashCode instead.

fabriciofx · 2019-11-16T18:02:18Z

src/main/java/org/cactoos/collection/Skipped.java

-     */
-    public Skipped(final int skip, final Collection<T> src) {
-        super(() -> new CollectionOf<>(
+        super(new Sticky<>(


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

fabriciofx · 2019-11-16T18:02:27Z

src/main/java/org/cactoos/collection/Sliced.java

-        super(() -> new CollectionOf<T>(
-            new org.cactoos.iterable.Sliced<T>(start, count, collection)
+    public Sliced(final int start, final int count, final Iterable<T> src) {
+        super(new Sticky<>(


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

fabriciofx · 2019-11-16T18:03:36Z

src/main/java/org/cactoos/collection/TailOf.java

-     */
-    public TailOf(final int num, final Collection<T> src) {
-        super(() -> new CollectionOf<T>(
+        super(new Sticky<T>(


@victornoel please, use the same indentation here which you used bellow (IMHO it's better).

victornoel · 2019-11-16T18:25:17Z

@fabriciofx it's done

fabriciofx

@victornoel please, take look one more time.

fabriciofx · 2019-11-16T18:46:54Z

src/main/java/org/cactoos/collection/CollectionOf.java

+
+    @Override
+    public int hashCode() {
+        return new HashCode(this).value();


@victornoel I don't think it the correct way to use HashCode. You must pass the attributes to the constructor, right? WDYT?

@fabriciofx I've modified HashCode so that it can take an Iterable of attributes, so this is an Iterable of the elements of the collection.

@victornoel sounds good to me

fabriciofx · 2019-11-16T20:25:20Z

@victornoel it seems fine to me. @paulodamaso can you merge it?

paulodamaso · 2019-11-18T18:34:03Z

@victornoel Therew are conflicts, please take a look

victornoel · 2019-11-18T19:10:59Z

@paulodamaso thank you, it should be fixed

paulodamaso · 2019-11-18T19:34:18Z

@rultor merge

rultor · 2019-11-18T19:36:37Z

@rultor merge

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

rultor · 2019-11-18T19:49:39Z

@rultor merge

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

0crat · 2019-11-18T20:06:33Z

@sereshqua /z please review this job completed by @fabriciofx /z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

0crat · 2019-11-18T20:07:42Z

Code review was too long (16 days), architects (@paulodamaso) were penalized, see §55

0crat · 2019-11-18T20:08:53Z

The job #1216 is now out of scope

0crat · 2019-11-18T20:09:33Z

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

sereshqua · 2019-11-18T20:19:17Z

@0crat quality good

0crat · 2019-11-18T20:27:29Z

Order was finished, quality is "good": +20 point(s) just awarded to @fabriciofx /z

0crat · 2019-11-18T20:30:09Z

Quality review completed: +4 point(s) just awarded to @sereshqua /z

victornoel mentioned this pull request Nov 2, 2019

CollectionEnvelope.java:43-47: CollectionEnvelope should... #1184

Closed

0crat added the scope label Nov 2, 2019

fabriciofx suggested changes Nov 12, 2019

View reviewed changes

victornoel force-pushed the 1184-collection-envelope branch from 801c5cd to 5172e56 Compare November 16, 2019 13:19

victornoel force-pushed the 1184-collection-envelope branch from 5172e56 to 789899a Compare November 16, 2019 16:59

fabriciofx suggested changes Nov 16, 2019

View reviewed changes

victornoel force-pushed the 1184-collection-envelope branch from 789899a to 4a54e9a Compare November 16, 2019 18:25

fabriciofx suggested changes Nov 16, 2019

View reviewed changes

(yegor256#1184) CollectionEnvelope is just an envelope

95f5a04

victornoel force-pushed the 1184-collection-envelope branch from 4a54e9a to 95f5a04 Compare November 18, 2019 19:10

rultor merged commit 95f5a04 into yegor256:master Nov 18, 2019

0crat removed the scope label Nov 18, 2019

victornoel deleted the 1184-collection-envelope branch February 15, 2020 14:07

(#1184) CollectionEnvelope is just an envelope #1216

(#1184) CollectionEnvelope is just an envelope #1216

Conversation

victornoel commented Nov 2, 2019

0crat commented Nov 2, 2019

codecov-io commented Nov 2, 2019 • edited Loading

Codecov Report

0crat commented Nov 2, 2019

paulodamaso commented Nov 6, 2019

victornoel commented Nov 6, 2019

fanifieiev commented Nov 6, 2019

fabriciofx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanifieiev Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victornoel commented Nov 16, 2019

fabriciofx commented Nov 16, 2019

victornoel commented Nov 16, 2019

fabriciofx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victornoel commented Nov 16, 2019

fabriciofx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriciofx commented Nov 16, 2019

paulodamaso commented Nov 18, 2019

victornoel commented Nov 18, 2019

paulodamaso commented Nov 18, 2019

rultor commented Nov 18, 2019

rultor commented Nov 18, 2019

0crat commented Nov 18, 2019

0crat commented Nov 18, 2019

0crat commented Nov 18, 2019

0crat commented Nov 18, 2019

sereshqua commented Nov 18, 2019

0crat commented Nov 18, 2019

0crat commented Nov 18, 2019

codecov-io commented Nov 2, 2019 •

edited

Loading

fanifieiev Nov 12, 2019 •

edited

Loading