#802 created iterator of floats and ints #832

krzyk · 2018-05-07T20:47:00Z

created IteratorOfFloats and IteratorOfInts and updated the appropriate Iterables
added tests for those new classes
added todo to continue implementation of longs and shorts

0crat · 2018-05-07T20:47:29Z

Job #832 is now in scope, role is REV

0crat · 2018-05-07T20:49:42Z

This pull request #832 is assigned to @proshin-roman /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

proshin-roman

@krzyk I found a few issues - please, check if they should/can be fixed.

proshin-roman · 2018-05-07T21:55:41Z

src/main/java/org/cactoos/iterator/IteratorOfFloats.java

+    /**
+     * The list of items to iterate.
+     */
+    private final float[] list;


@krzyk As this field is not a list anyway, wouldn't it better to rename it to items?

proshin-roman · 2018-05-07T21:56:01Z

src/main/java/org/cactoos/iterator/IteratorOfInts.java

+    /**
+     * The list of items to iterate.
+     */
+    private final int[] list;


@krzyk same here: rename it to items?

proshin-roman · 2018-05-07T21:58:32Z

src/test/java/org/cactoos/iterator/IteratorOfFloatsTest.java

+    @Test
+    public void emptyIteratorDoesNotHaveNext() {
+        MatcherAssert.assertThat(
+            "Can't create empty iterator",


@krzyk The failure message looks strange - failing of the assertions means that empty iterator has the next item which is wrong. Could you adjust the message, please?

proshin-roman · 2018-05-07T21:59:28Z

src/test/java/org/cactoos/iterator/IteratorOfFloatsTest.java

+        MatcherAssert.assertThat(
+            "Can't create empty iterator",
+            new IteratorOfFloats().hasNext(),
+            CoreMatchers.equalTo(false)


@krzyk let's please use new IsEqual(true) instead of calling a static method?

proshin-roman · 2018-05-07T22:01:04Z

src/test/java/org/cactoos/iterator/IteratorOfFloatsTest.java

+    @Test
+    public void nonEmptyIteratorDoesNotHaveNext() {
+        MatcherAssert.assertThat(
+            "Can't create non empty iterator",


@krzyk here again: the failure message is not correct - it should explain the cause of assertion failing: an iterator with fetched items still has the next item which is wrong. Please, fix the message.

proshin-roman · 2018-05-07T22:01:28Z

src/test/java/org/cactoos/iterator/IteratorOfFloatsTest.java

+        MatcherAssert.assertThat(
+            "Can't create non empty iterator",
+            this.iteratorWithFetchedElements().hasNext(),
+            CoreMatchers.equalTo(false)


@krzyk And here too: please, use IsEqual

proshin-roman · 2018-05-07T22:02:43Z

src/test/java/org/cactoos/iterator/IteratorOfIntsTest.java

+ * @since 0.32
+ * @checkstyle JavadocMethodCheck (500 lines)
+ */
+public final class IteratorOfIntsTest {


@krzyk all comments written for IteratorOfFloatsTest can be applied for this class as well.

proshin-roman · 2018-05-07T22:09:52Z

src/main/java/org/cactoos/iterable/IterableOfInts.java

-            return iterable;
-        });
+        super(() -> ()
+            -> new UncheckedScalar<>(() -> new IteratorOfInts(values)).value()


@krzyk I might be wrong here (please, correct me if it's so), but using of UncheckedScalar looks redundant here, as the super ctor already wraps the argument into UncheckedScalar:

cactoos/src/main/java/org/cactoos/iterable/IterableEnvelope.java

Line 54 in 5ead4ac

this.iterable = new UncheckedScalar<>(scalar);

Could you check please if that's correct and UncheckedScalar can be ommitted here?

@proshin-roman unchecked scalar is for Iterator interface (there are 3 lambdas here) so I need to wrap it at the bottom, wrapping in IterableEnvelope doesn't help here

@krzyk why didn't you just write () -> () -> new IteratorOfInts(values)? I don't get what you wrote provides. The constructor of IteratorOfInts does not throw any exception...

@krzyk or () -> new IterableOf<>(new IteratorOfInts(values)) if you prefer

@victornoel I didn't know I could (regarding the first of your comments - it wasn't possible, I didn't tried the second one)

@krzyk ok thanks, I will take that into account for solving #838 :)

codecov-io · 2018-05-08T05:26:48Z

Codecov Report

Merging #832 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #832      +/-   ##
============================================
+ Coverage     84.51%   84.56%   +0.04%     
- Complexity     1368     1376       +8     
============================================
  Files           249      251       +2     
  Lines          3695     3705      +10     
  Branches        215      213       -2     
============================================
+ Hits           3123     3133      +10     
  Misses          524      524              
  Partials         48       48

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/org/cactoos/iterable/IterableOfLongs.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...in/java/org/cactoos/iterator/IteratorOfFloats.java	`100% <100%> (ø)`	`5 <5> (?)`
...main/java/org/cactoos/iterator/IteratorOfInts.java	`100% <100%> (ø)`	`5 <5> (?)`
...in/java/org/cactoos/iterable/IterableOfFloats.java	`100% <100%> (ø)`	`2 <2> (-1)`	⬇️
...main/java/org/cactoos/iterable/IterableOfInts.java	`100% <100%> (ø)`	`2 <2> (-1)`	⬇️

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 4738a00...9346b81. Read the comment docs.

krzyk · 2018-05-08T05:26:54Z

@proshin-roman I've updated the code, please take a look

proshin-roman

@llorllale good to merge

llorllale · 2018-05-08T13:20:32Z

@rultor merge

rultor · 2018-05-08T13:21:29Z

@rultor merge

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

rultor · 2018-05-08T13:31:42Z

@rultor merge

@llorllale Done! FYI, the full log is here (took me 10min)

0crat · 2018-05-08T13:57:30Z

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

0crat · 2018-05-08T13:59:02Z

The job #832 is now out of scope

elenavolokhova · 2018-05-08T19:22:39Z

@0crat quality good

0crat · 2018-05-08T20:41:06Z

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

0crat · 2018-05-08T20:41:11Z

Quality review completed: +8 point(s) just awarded to @elenavolokhova /z

yegor256#802 created iterator of floats and ints

d505624

0crat added the scope label May 7, 2018

proshin-roman suggested changes May 7, 2018

View reviewed changes

yegor256#802 CR

9346b81

proshin-roman approved these changes May 8, 2018

View reviewed changes

rultor merged commit 9346b81 into yegor256:master May 8, 2018

0crat removed the scope label May 8, 2018

krzyk mentioned this pull request May 8, 2018

IterableOfFloats.java:35-40: Introduce IteratorOfFloats,... #802

Closed

victornoel mentioned this pull request May 10, 2018

IteratorOfLongs and IteratorOfShorts #854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#802 created iterator of floats and ints #832

#802 created iterator of floats and ints #832

krzyk commented May 7, 2018

0crat commented May 7, 2018

0crat commented May 7, 2018

proshin-roman left a comment

proshin-roman May 7, 2018

proshin-roman May 7, 2018

proshin-roman May 7, 2018

proshin-roman May 7, 2018

proshin-roman May 7, 2018

proshin-roman May 7, 2018

proshin-roman May 7, 2018

proshin-roman May 7, 2018

krzyk May 8, 2018

victornoel May 8, 2018

victornoel May 8, 2018

krzyk May 8, 2018

victornoel May 8, 2018

codecov-io commented May 8, 2018 •

edited

Loading

krzyk commented May 8, 2018

proshin-roman left a comment

llorllale commented May 8, 2018

rultor commented May 8, 2018

rultor commented May 8, 2018

0crat commented May 8, 2018

0crat commented May 8, 2018

elenavolokhova commented May 8, 2018

0crat commented May 8, 2018

0crat commented May 8, 2018

#802 created iterator of floats and ints #832

#802 created iterator of floats and ints #832

Conversation

krzyk commented May 7, 2018

0crat commented May 7, 2018

0crat commented May 7, 2018

proshin-roman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 8, 2018 • edited Loading

Codecov Report

krzyk commented May 8, 2018

proshin-roman left a comment

Choose a reason for hiding this comment

llorllale commented May 8, 2018

rultor commented May 8, 2018

rultor commented May 8, 2018

0crat commented May 8, 2018

0crat commented May 8, 2018

elenavolokhova commented May 8, 2018

0crat commented May 8, 2018

0crat commented May 8, 2018

codecov-io commented May 8, 2018 •

edited

Loading