Reimplemented Collections.scan methods #1684

ashrwin · 2016-11-17T05:59:21Z

codecov-io · 2016-11-17T06:19:54Z

Current coverage is 97.01% (diff: 100%)

Merging #1684 into master will decrease coverage by <.01%

@@             master      #1684   diff @@
==========================================
  Files            89         89          
  Lines         11264      11243    -21   
  Methods           0          0          
  Messages          0          0          
  Branches       1878       1877     -1   
==========================================
- Hits          10928      10907    -21   
  Misses          196        196          
  Partials        140        140

Powered by Codecov. Last update 2625b10...c8c3ef5

danieldietrich

Hi @ashrko619,
thank you, well done - nice and clean!
Because we are already on this topic, could you please change the method I mentioned?
Many thanks!
Daniel

danieldietrich · 2016-11-17T12:22:12Z

javaslang/src/main/java/javaslang/collection/CharSeq.java

@@ -809,13 +809,13 @@ public CharSeq reverse() {
    @Override
    public <U> IndexedSeq<U> scanLeft(U zero, BiFunction<? super U, ? super Character, ? extends U> operation) {
        Objects.requireNonNull(operation, "operation is null");
-        return Collections.scanLeft(this, zero, operation, Vector.empty(), Vector::append, Function.identity());
+        return Collections.scanLeft(this, zero, operation, Iterator::toVector);


I've recognized one thing that is outside (but related) to this PR: CharSeq.scan() currently returns IndexedSeq<Character> but it should return CharSeq.

Could you please change the return type of scan() to CharSeq?

@Override public IndexedSeq<Character> scan(Character zero, BiFunction<? super Character, ? super Character, ? extends Character> operation) { return Collections.scanLeft(this, zero, operation, Iterator::toCharSeq); }

danieldietrich · 2016-11-17T12:25:09Z

javaslang/src/main/java/javaslang/collection/PriorityQueue.java

    }

    @Override
    public <U> PriorityQueue<U> scanLeft(U zero, BiFunction<? super U, ? super T, ? extends U> operation) {
        Objects.requireNonNull(operation, "operation is null");
-        return Collections.scanLeft(this, zero, operation, empty(naturalComparator()), PriorityQueue::enqueue, Function.identity());
+


minor: unnecessary newline

danieldietrich · 2016-11-17T12:43:29Z

javaslang/src/main/java/javaslang/collection/CharSeq.java

@@ -803,7 +803,7 @@ public CharSeq reverse() {

    @Override
    public IndexedSeq<Character> scan(Character zero, BiFunction<? super Character, ? super Character, ? extends Character> operation) {
-        return scanLeft(zero, operation);
+        return Collections.scanLeft(this, zero, operation, Iterator::toCharSeq);


Awesome, that was fast :-)
Can the method return CharSeq instead of IndexedSeq<Character>?

@danieldietrich: Ran into a bug, have opened an issue #1685

danieldietrich · 2016-11-20T11:37:44Z

@ashrko619 because of ongoing PRs the branch conflicts need to be resolved. With your fix of #1685 the Iterator.toCharSeq method should work now.

As soon as you merged your local branch I will pull this PR! Thank you :)

l0rinc · 2016-11-20T13:08:57Z

javaslang/src/main/java/javaslang/collection/Collections.java

-            final Seq<? extends T> result = (iterable instanceof Seq) ? (Seq<T>) iterable
-                                                                      : Vector.ofAll(iterable);
-            return result.reverseIterator();
+            return Iterator.ofAll(iterable).foldLeft(List.<T>empty(), List::prepend).iterator();


Good idea to use a stack here instead of a Vector (which would require a size hint).
I suggest you use pushAll instead of foldLeft:

List.<T> empty().pushAll(iterable).iterator();

Good idea to use a stack here instead of a Vector (which would require a size hint).
Can you explain ? 😄

you're using a stack's iterator here (i.e. List is a Stack), which is basically a reverse iterator.
It's a better solution to the problem, than what I've written previously, i.e. create a Vector from it and traverse it backwards.

l0rinc · 2016-11-20T13:09:58Z

javaslang/src/main/java/javaslang/collection/Maps.java

        Objects.requireNonNull(operation, "operation is null");
-        return Collections.scanLeft(map, zero, operation, emptySupplier.get(), Maps::put, Function.identity());
+        return (M) Collections.scanLeft(map, zero, operation, finisher);


hmmm, why wasn't a cast needed before?

Am also not sure why it wasn't needed 🤔

l0rinc · 2016-11-20T13:11:27Z

javaslang/src/main/java/javaslang/collection/TreeSet.java

@@ -779,20 +779,17 @@ public int length() {
    @Override
    public TreeSet<T> scan(T zero, BiFunction<? super T, ? super T, ? extends T> operation) {
        Objects.requireNonNull(operation, "operation is null");
-        return Collections.scanLeft(this, zero, operation, TreeSet.empty(comparator()), TreeSet::add, Function.identity());
+        return Collections.scanLeft(this, zero, operation, it -> TreeSet.ofAll(comparator(), it));


it = iterator?
if you want to use an abbreviation, please consider using iter, it's more commonly used for iterator

l0rinc · 2016-11-20T13:15:46Z

javaslang/src/main/java/javaslang/collection/Collections.java

+    static <T, U, R extends Traversable<U>> R scanRight(Traversable<? extends T> traversable,
+                                                        U zero, BiFunction<? super T, ? super U, ? extends U> operation, Function<Iterator<U>, R> finisher) {
+
+        final Iterator<? extends T> reversedElements = reverseIterator(traversable.iterator());


👍 for using the new reverseIterator

Why do you need the .iterator() exactly, reverseIterator(traversable) should work also?

that's right

l0rinc · 2016-11-20T14:42:39Z

To make this mergeable, please rebase on master
Only push, once all tests are passing
Commits should contain coherent units of work. Please reorder you commits logically, not incidentally, i.e. if you've made a mistake in your previous commit, edit that one and force push, instead of creating many altering commits - it helps the reviewer.

ashrwin · 2016-11-21T14:14:27Z

javaslang/src/main/java/javaslang/collection/Map.java

+     * @return a new map with the same keySet but transformed values.
+     */
+    Map<K, V> replaceAll(BiFunction<? super K, ? super V, ? extends V> function);
+


I merged the changes but am not sure why these are added as part of this commit 🙈

Mmhh, today I did a push --force on a branch/PR that wasn't merged yet. I think something went wrong o_O.

Don't know how this can be repaired 🙈 However, the current master contains these lines already: https://github.com/javaslang/javaslang/blob/master/javaslang/src/main/java/javaslang/collection/Map.java#L677-L703

@paplorinc is there some git magic that can be applied here? E.g. rebasing / rewriting this PR ...

@ashrko619 don't merge, rebase :)
From Idea you can do that from the VCS -> Git -> rebase my github fork, and solve the conflicts

When I try to rebase there are no conflicts now 😕 Should I rollback to an older commit and try ?

danieldietrich · 2016-11-23T08:27:38Z

Well done @ashrko619, many thanks!

danieldietrich suggested changes Nov 17, 2016

View reviewed changes

danieldietrich added design/refactoring/improvement «vavr-collection» labels Nov 17, 2016

danieldietrich added this to the 2.1.0 milestone Nov 17, 2016

danieldietrich reviewed Nov 17, 2016

View reviewed changes

danieldietrich mentioned this pull request Nov 17, 2016

Iterator.toCharSeq() error #1685

Closed

l0rinc reviewed Nov 20, 2016

View reviewed changes

ashrwin added 3 commits November 21, 2016 13:55

reimplemented Collections.scan methods

1b406a4

fixed formatting errors

06fb27c

refactored CharSeq.scan to return CharSeq

f4439a7

ashrwin force-pushed the scan-methods branch from b6cc582 to 2548f3a Compare November 21, 2016 14:08

ashrwin commented Nov 21, 2016

View reviewed changes

merged changes and refactored as per suggestions

c8c3ef5

ashrwin force-pushed the scan-methods branch from 2548f3a to c8c3ef5 Compare November 21, 2016 14:36

danieldietrich approved these changes Nov 23, 2016

View reviewed changes

danieldietrich merged commit 765071b into vavr-io:master Nov 23, 2016

danieldietrich mentioned this pull request Nov 23, 2016

Re-implement Collections.scanLeft/scanRight: #1610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplemented Collections.scan methods #1684

Reimplemented Collections.scan methods #1684

ashrwin commented Nov 17, 2016

codecov-io commented Nov 17, 2016 •

edited

Loading

danieldietrich left a comment

danieldietrich Nov 17, 2016

danieldietrich Nov 17, 2016

danieldietrich Nov 17, 2016

ashrwin Nov 17, 2016

danieldietrich commented Nov 20, 2016

l0rinc Nov 20, 2016 •

edited

Loading

ashrwin Nov 21, 2016

l0rinc Nov 21, 2016

l0rinc Nov 20, 2016

ashrwin Nov 21, 2016

l0rinc Nov 20, 2016 •

edited

Loading

l0rinc Nov 20, 2016

l0rinc Nov 20, 2016

danieldietrich Nov 20, 2016

l0rinc commented Nov 20, 2016

ashrwin Nov 21, 2016

danieldietrich Nov 21, 2016 •

edited

Loading

l0rinc Nov 21, 2016

ashrwin Nov 22, 2016

danieldietrich commented Nov 23, 2016

Reimplemented Collections.scan methods #1684

Reimplemented Collections.scan methods #1684

Conversation

ashrwin commented Nov 17, 2016

codecov-io commented Nov 17, 2016 • edited Loading

Current coverage is 97.01% (diff: 100%)

danieldietrich 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

danieldietrich commented Nov 20, 2016

l0rinc Nov 20, 2016 • 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

l0rinc Nov 20, 2016 • 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

l0rinc commented Nov 20, 2016

Choose a reason for hiding this comment

danieldietrich Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietrich commented Nov 23, 2016

codecov-io commented Nov 17, 2016 •

edited

Loading

l0rinc Nov 20, 2016 •

edited

Loading

l0rinc Nov 20, 2016 •

edited

Loading

danieldietrich Nov 21, 2016 •

edited

Loading