list-ops: add to track #207
Conversation
|
Poke! |
|
Marry Christmas, John.
…On Sat, Dec 24, 2016 at 6:38 PM, John Ryan ***@***.***> wrote:
Poke!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#207 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIezhyx4QSBA9L1rnejJdFefp6kIyFjwks5rLazbgaJpZM4LHxsk>
.
|
|
Estimated arrival of this code is first Days of January. :)
On Sun, Dec 25, 2016 at 11:49 AM, Dmitry Noranovich <[email protected]>
wrote:
… Marry Christmas, John.
On Sat, Dec 24, 2016 at 6:38 PM, John Ryan ***@***.***>
wrote:
> Poke!
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#207 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AIezhyx4QSBA9L1rnejJdFefp6kIyFjwks5rLazbgaJpZM4LHxsk>
> .
>
|
|
Merry Christmas, @javaeeeee! 🎄 ...and Happy (Belated) Boxing Day!
Sounds lovely, sir. Happy New Year! 🍾 🎆 |
|
Hello! Thanks for congratulations, @jtigger! Sorry for the delay. This implementation is inspired by the one in the C# track. Added some tests. The example implementation is the simplest possible. |
There was a problem hiding this comment.
Great stuff, Dmitry!
Bunch of comments and questions inline.
Generics
Clearly this exercise is going to appear late in the game. We're still hashing out how we'll introduce generics (please acquaint and chime in on #230).
Seems like we should either not use type variables or include test cases (or modify existing test cases) that exercise those variables.
append()
I'm really struggling with this operation:
- it gets away from the focus on immutability which I'm really loving;
- it's redundant in functionality with
concat()just with a different interface. It's not just "bad" API design, it means a chunk of the exercise feels a bit like busy work.
|
|
||
| @Test | ||
| @Ignore | ||
| public void fileteredEmptyListShouldBeAnEmptyList() { |
There was a problem hiding this comment.
typo: s/filetered/filtered/
There was a problem hiding this comment.
Corrected and renamed. Thank you. 🥇
| = Collections.emptyList(); | ||
|
|
||
| @Test | ||
| public void lengthOfAnEmptyListShouldBeZero() { |
There was a problem hiding this comment.
Here (and in mapOfAnEmptyListShouldBeAnEmptyList(), fileteredEmptyListShouldBeAnEmptyList()), the subject becomes a property of the list whereas in the other tests, the subject is the list itself.
🤔
Genuine question: is it "better" to have consistent test method naming? or to have a variety to break up the monotony that comes with consistency??! :)
There was a problem hiding this comment.
Renamed. Just failed to come up with consistent names on the first pass. :)
| @Test | ||
| @Ignore | ||
| public void shouldReturnCorrectLengthOfAnNonEmptyList() { | ||
| final int expected = 4; |
There was a problem hiding this comment.
🤔
Thinking out loud: I find that the greater the space between definition and use, the "harder" it is to read. What do you think of moving L34 to L41?
There was a problem hiding this comment.
Moved. Just stuck to habitual code conventions.
| public void shouldReverseAnEmptyList() { | ||
| final List<Integer> actual = ListOps.reverse(EMPTY_LIST); | ||
|
|
||
| assertNotNull(actual); |
There was a problem hiding this comment.
👍
I've said it before, but I really appreciate how little bits like this smooth-out the TDD experience: the more we keep folks in test failures (rather than test errors), the smoother the experience.
| .collect(Collectors.toList()); | ||
| Collections.reverse(reversedList); | ||
| final List<Integer> expected | ||
| = Collections.unmodifiableList(reversedList); |
There was a problem hiding this comment.
This exercise is clearly going to be placed late in the track, so addressing 2nd order topics like leaning toward immutability seems appropriate. But let's talk about it.
😃
It's particularly important in this exercise to hand in immutable input into ListOps! Love that this is your go-to.
🤔
How can we make the case for making the expected value immutable?
There was a problem hiding this comment.
Seems that it's unnecessary as we don't pass it anywhere. That's an example where code reviews really shine. 👍
|
|
||
| public static <T> List<T> map(final List<T> list, | ||
| UnaryOperator<T> mapper) { | ||
| return list.stream().map(mapper).collect(Collectors.toList()); |
There was a problem hiding this comment.
hahaha!! Cheater! 😆
👍
I joke, but actually, relying on the JSL for this functionality gives me great confidence that the tests are solid. Not sure if that was your intention, but great move!
There was a problem hiding this comment.
Tried to keep it simple. The example is needed only to test the test. No pun intended. :)
| return list.stream().filter(predicate).collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public static <U, T> U reduce(final List<T> list, |
There was a problem hiding this comment.
👍
I love how this mirrors exactly the interface for streams... what a great way to introduce this part of the API!
There was a problem hiding this comment.
Thank you. It makes sense to expose Java API here like the C# one was used in the C# track
| = Stream.concat(list1.stream(), list2.stream()) | ||
| .collect(Collectors.toList()); | ||
| Collections.sort(sortedCancatenatedList); | ||
| final List<Integer> expected = Collections.unmodifiableList( |
There was a problem hiding this comment.
🤔
It can significantly increase the readability of a test when the expectation is as close to "literal" as possible. Otherwise, the reader must apply the operations in their head and visualize the result.
For example:
final List<Integer> expected = Collections.unmodifiableList(
IntStream.range(0, 4, 1, 6).boxed().collect(Collectors.toList())
);There's another example of this in reduceShouldConcatenateTwoNonEmptyLists(), below 👇 .
There was a problem hiding this comment.
Seems that it also can be modifiable here.
There was a problem hiding this comment.
Otherwise, didn't touch the code. Lists to concatenate are used both to create the result and to test the concat() method. Could you take a look again here and elaborate more, please.
| public void shouldReturnIdentityWhenAnEmptyListIsReduced() { | ||
| final int expected = 0; | ||
| final int actual | ||
| = ListOps.reduce(EMPTY_LIST, 0, (x, y) -> x + y, Integer::sum); |
There was a problem hiding this comment.
👍
Nice touch! I love how you're seeking in little bits of the Java FP stuff in! (Here, both the method reference and that there's a set of functions ready for use!)
| final List<Integer> sortedCancatenatedList | ||
| = Stream.concat(list1.stream(), list2.stream()) | ||
| .collect(Collectors.toList()); | ||
| Collections.sort(sortedCancatenatedList); |
There was a problem hiding this comment.
🤔
Concatenation is about connecting a set of things, in series. Should we be sorting?
There was a problem hiding this comment.
On the second pass, it looks like it could be even harmful. :) Thank you.
There was a problem hiding this comment.
Both sorts were unnecessary, so removing them sped up the test. Overengineered for lists containing same elements.
|
Thanks for the thorough review of the code, @jtigger, liked it. Appreciate
your humor. Changes and replies are coming soon! :)
…On Wed, Jan 11, 2017 at 12:15 PM, John Ryan ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Great stuff, Dmitry!
Bunch of comments and questions inline.
Generics
Clearly this exercise is going to appear late in the game. We're still
hashing out how we'll introduce generics (please acquaint and chime in on
#230 <#230>).
Seems like we should either not use type variables *or* include test
cases (or modify existing test cases) that exercise those variables.
append()
I'm really struggling with this operation:
- it gets away from the focus on immutability which I'm really loving;
- it's redundant in functionality with concat() just with a different
interface. It's not just "bad" API design, it means a chunk of the exercise
feels a bit like busy work.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + @ignore
+ public void shouldMapNonEmptyList() {
+ final List<Integer> expected
+ = Collections.unmodifiableList(Arrays.asList(2, 4, 6, 8));
+ final List<Integer> list
+ = Collections.unmodifiableList(Arrays.asList(1, 3, 5, 7));
+ final List<Integer> actual = ListOps.map(list, x -> x + 1);
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void fileteredEmptyListShouldBeAnEmptyList() {
typo: s/filetered/filtered/
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +import java.util.stream.IntStream;
+import java.util.stream.Stream;
+import static junit.framework.TestCase.assertEquals;
+import static junit.framework.TestCase.assertFalse;
+import static junit.framework.TestCase.assertNotNull;
+import static junit.framework.TestCase.assertTrue;
+import org.junit.Ignore;
+import org.junit.Test;
+
+public class ListOpsTest {
+
+ private static final List<Integer> EMPTY_LIST
+ = Collections.emptyList();
+
+ @test
+ public void lengthOfAnEmptyListShouldBeZero() {
Here (and in mapOfAnEmptyListShouldBeAnEmptyList(),
fileteredEmptyListShouldBeAnEmptyList()), the subject becomes a property
of the list whereas in the other tests, the subject is the list itself.
🤔
Genuine question: is it "better" to have consistent test method naming? or
to have a variety to break up the monotony that comes with consistency??! :)
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ private static final List<Integer> EMPTY_LIST
+ = Collections.emptyList();
+
+ @test
+ public void lengthOfAnEmptyListShouldBeZero() {
+ final int expected = 0;
+ final int actual = ListOps.length(EMPTY_LIST);
+
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReturnCorrectLengthOfAnNonEmptyList() {
+ final int expected = 4;
🤔
Thinking out loud: I find that the greater the space between definition
and use, the "harder" it is to read. What do you think of moving L34 to L41?
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, expected)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final int actual = ListOps.length(list);
+
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReverseAnEmptyList() {
+ final List<Integer> actual = ListOps.reverse(EMPTY_LIST);
+
+ assertNotNull(actual);
👍
I've said it before, but I really appreciate how little bits like this
smooth-out the TDD experience: the more we keep folks in test failures
(rather than test errors), the smoother the experience.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + assertNotNull(actual);
+ assertTrue(actual.isEmpty());
+ }
+
+ @test
+ @ignore
+ public void shouldReverseNonEmptyList() {
+ final int length = 100;
+ final int startValue = 0;
+ final List<Integer> reversedList
+ = IntStream.range(startValue, length)
+ .boxed()
+ .collect(Collectors.toList());
+ Collections.reverse(reversedList);
+ final List<Integer> expected
+ = Collections.unmodifiableList(reversedList);
This exercise is clearly going to be placed late in the track, so
addressing 2nd order topics like leaning toward immutability seems
appropriate. But let's talk about it.
😃
It's particularly important in this exercise to hand in immutable input
into ListOps! Love that this is your go-to.
🤔
How can we make the case for making the *expected* value immutable?
------------------------------
In exercises/list-ops/src/example/java/ListOps.java
<#207 (review)>:
> + private ListOps() {
+ }
+
+ public static <T> int length(final List<T> list) {
+ return list.size();
+ }
+
+ public static <T> List<T> reverse(final List<T> list) {
+ List<T> result = new ArrayList(list);
+ Collections.reverse(result);
+ return result;
+ }
+
+ public static <T> List<T> map(final List<T> list,
+ UnaryOperator<T> mapper) {
+ return list.stream().map(mapper).collect(Collectors.toList());
hahaha!! Cheater! 😆
👍
I joke, but actually, relying on the JSL for this functionality gives me
great confidence that the tests are solid. Not sure if that was your
intention, but great move!
------------------------------
In exercises/list-ops/src/example/java/ListOps.java
<#207 (review)>:
> + List<T> result = new ArrayList(list);
+ Collections.reverse(result);
+ return result;
+ }
+
+ public static <T> List<T> map(final List<T> list,
+ UnaryOperator<T> mapper) {
+ return list.stream().map(mapper).collect(Collectors.toList());
+ }
+
+ public static <T> List<T> filter(final List<T> list,
+ Predicate<T> predicate) {
+ return list.stream().filter(predicate).collect(Collectors.toList());
+ }
+
+ public static <U, T> U reduce(final List<T> list,
👍
I love how this mirrors *exactly* the interface for streams... what a
great way to introduce this part of the API!
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + }
+
+ @test
+ @ignore
+ public void shouldConcatenateTwoListsWithSameElements() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(1, 6).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> sortedCancatenatedList
+ = Stream.concat(list1.stream(), list2.stream())
+ .collect(Collectors.toList());
+ Collections.sort(sortedCancatenatedList);
+ final List<Integer> expected = Collections.unmodifiableList(
🤔
It can significantly increase the readability of a test when the
expectation is as close to "literal" as possible. Otherwise, the reader
must apply the operations in their head and visualize the result.
For example:
final List<Integer> expected = Collections.unmodifiableList(
IntStream.range(0, 4, 1, 6).boxed().collect(Collectors.toList())
);
There's another example of this in reduceShouldConcatenateTwoNonE
mptyLists(), below 👇 .
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + );
+ final List<Integer> actual
+ = ListOps.concat(list1, list2, EMPTY_LIST, list3, list4);
+ Collections.sort(actual);
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReturnIdentityWhenAnEmptyListIsReduced() {
+ final int expected = 0;
+ final int actual
+ = ListOps.reduce(EMPTY_LIST, 0, (x, y) -> x + y, Integer::sum);
👍
Nice touch! I love how you're seeking in little bits of the Java FP stuff
in! (Here, both the method reference *and* that there's a set of
functions ready for use!)
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateTwoListsWithSameElements() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(1, 6).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> sortedCancatenatedList
+ = Stream.concat(list1.stream(), list2.stream())
+ .collect(Collectors.toList());
+ Collections.sort(sortedCancatenatedList);
🤔
Concatenation is about connecting a set of things, *in series*. Should we
be sorting?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIezh7WSoJO6zvm4NH8-uHmrNFXOgvcbks5rRQ4YgaJpZM4LHxsk>
.
|
|
For all tests removed unnecessary unmodifiable for expected lists and placed expected results closer to assertions. As to the append() method, it's not difficult to remove it from the exercise. An idea why it could stay here is that while it can be bad for production code for the reasons you mentioned, but here, in an exercise, it could prod a learner to write some additional lines of code. Anyway, your decision is needed here about what to do with this code. |
|
As to the topic of Generics seems that we should use it because it's an important language feature. As to the tests, the C# track has tests for various data types only for the reduce operation. The example the Java track uses the most complex form of the reduce() method because it can reduce the list of Integers into an Integer (sum) and into a List of Integers (concatenation) and there are tests for that. Also, the C# has the tests for reduce for Strings, that were not added to the Java version to speed up the release of the exercise. It's not difficult to add tests using various data types and that could explain to a learner why Generics can simplify the implementation. |
|
@javaeeeee said:
Well, I've convinced myself that my idea is great! 😆 No, seriously, if you're actually on the fence, then my vote is to remove I'm evaluating the additional lines of code that |
|
Hey @javaeeeee I meant to get to your PR, this morning. The work day is getting started, so I'm gonna have to kick it to tomorrow. Sorry for the delay. |
There was a problem hiding this comment.
Again, sorry for the delay, Dmitry.
We're definitely making progress!
I seem to be really harping on making test code as simple as possible. In particular, setting up expected values as literally as possible.
I'm painfully aware that my feedback is rather pedantic and particularly opinionated. I hope that I'm doing a good job of motivating these changes.
At the heart of it a desire to create a delightful experience for our users. Not unlike in literature, refinement is often about seeking concise ways of expressing an idea.
It's a kind of empathy to put ourselves in the shoes of someone learning Java (and/or seeing these language features for the first time) and asking ourselves how much effort it would take to understand the problem/question/test.
Empathy skillfully conveyed lands as respect. It's that we took the time to minimize how much translation/interpretation we ask of our users to understand the question/problem we're posing. They may not consciously know it, but they'll feel the difference.
| @@ -53,23 +53,22 @@ public void shouldReverseAnEmptyList() { | |||
|
|
|||
| @Test | |||
| @Ignore | |||
| public void shouldReverseNonEmptyList() { | |||
| public void shouldReverseANonEmptyList() { | |||
| final int length = 100; | |||
There was a problem hiding this comment.
👍 extracting a value to a variable is a great move where the input and output need to agree.
🤔 this larger number forces us to use IntStream.range() to generate the list (and applying an operation on top of it; here sorting). If we start with a smaller value, we could start with the transformed value.
Example:
final List<Integer> list = Arrays.asList(0, 1, 2, 3, 4, 5, 6, 7, 8);
final List<Integer> reversed = Arrays.asList(8, 7, 6, 5, 4, 3, 2, 1, 0);| final List<Integer> list = Collections.unmodifiableList( | ||
| IntStream.range(startValue, length) | ||
| .boxed() | ||
| .collect(Collectors.toList()) | ||
| ); | ||
| final List<Integer> actual | ||
| = ListOps.reverse(list); | ||
| final List<Integer> expected = reversedList; |
There was a problem hiding this comment.
🤔 what do you think of using reversedList as the expected value directly in the assert? I'm thinking this could increase the readability of the assert itself.
There was a problem hiding this comment.
Removed reversedList altogether.
| final List<Integer> list | ||
| = Collections.unmodifiableList(Arrays.asList(1, 3, 5, 7)); | ||
| final List<Integer> actual = ListOps.map(list, x -> x + 1); | ||
| final List<Integer> expected |
There was a problem hiding this comment.
🤔 (redundant suggestion) this "literal" (as close as we get in Java) is short enough that it could fit inline in the assert, below. The motivation, here, is to bring the declaration of the value as close to its use as possible (in this case, reducing that distance to zero).
| @@ -111,12 +110,11 @@ public void fileteredEmptyListShouldBeAnEmptyList() { | |||
| @Test | |||
| @Ignore | |||
| public void shouldFilterNonEmptyList() { | |||
| final List<Integer> expected | |||
| = Collections.unmodifiableList(Arrays.asList(1, 3)); | |||
| final List<Integer> list = Collections.unmodifiableList( | |||
There was a problem hiding this comment.
🤔 I think it is really important that we take the opportunity to show-off the Java Streams API as much as we can, here; using the IntStream.range() idiom being one of those. But in this specific case, as written, it feels a little awkward to use Arrays.asList() for a two-element list and then upshift to IntStream to create a four-element list.
💡 you could create a large input list and use a highly-selective function in filter() that yields a short list. For example, a list from [1, 21) where values are evenly divisible by 5?
There was a problem hiding this comment.
Created a longer list and introduced a filter. Extracted predicate for reuse.
| @@ -343,9 +325,8 @@ public void shouldConcatenateSeveralLists() { | |||
| sortedCancatenatedList.addAll(list3); | |||
| sortedCancatenatedList.addAll(list4); | |||
| Collections.sort(sortedCancatenatedList); | |||
There was a problem hiding this comment.
❓ I'm confused as to what's motivating the sort. I was expecting to see a few chunks that are, in series, appended. In fact, I'm expecting that the order is preserved. Makes me feel like I'm missing a subtle point.
There was a problem hiding this comment.
Now I see that it was unnecessary.
|
Hello, John,
Thank you for your feedback. I'll make changes ASAP.
…On Wed, Jan 18, 2017 at 11:21 AM, John Ryan ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Again, sorry for the delay, Dmitry.
We're definitely making progress!
I seem to be really harping on making test code as simple as possible. In
particular, setting up expected values as literally as possible.
I'm painfully aware that my feedback is rather pedantic and particularly
opinionated. I hope that I'm doing a good job of motivating these changes.
At the heart of it a desire to create a delightful experience for our
users. Not unlike in literature, refinement is often about seeking concise
ways of expressing an idea.
It's a kind of empathy to put ourselves in the shoes of someone learning
Java (and/or seeing these language features for the first time) and asking
ourselves how much effort it would take to understand the
problem/question/test.
Empathy skillfully conveyed lands as respect. It's that we took the time
to minimize how much translation/interpretation we ask of our users to
understand the question/problem we're posing. They may not consciously know
it, but they'll feel the difference.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> @@ -53,23 +53,22 @@ public void shouldReverseAnEmptyList() {
@test
@ignore
- public void shouldReverseNonEmptyList() {
+ public void shouldReverseANonEmptyList() {
final int length = 100;
👍 extracting a value to a variable is a great move where the input and
output need to agree.
🤔 this larger number forces us to use IntStream.range() to generate the
list (and applying an operation on top of it; here sorting). If we start
with a smaller value, we could *start* with the transformed value.
Example:
final List<Integer> list = Arrays.asList(0, 1, 2, 3, 4, 5, 6, 7, 8);final List<Integer> reversed = Arrays.asList(8, 7, 6, 5, 4, 3, 2, 1, 0);
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> final List<Integer> list = Collections.unmodifiableList(
IntStream.range(startValue, length)
.boxed()
.collect(Collectors.toList())
);
final List<Integer> actual
= ListOps.reverse(list);
+ final List<Integer> expected = reversedList;
🤔 what do you think of using reversedList *as* the expected value
directly in the assert? I'm thinking this could increase the readability of
the assert itself.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> final List<Integer> list
= Collections.unmodifiableList(Arrays.asList(1, 3, 5, 7));
final List<Integer> actual = ListOps.map(list, x -> x + 1);
+ final List<Integer> expected
🤔 (redundant suggestion) this "literal" (as close as we get in Java) is
short enough that it *could* fit inline in the assert, below. The
motivation, here, is to bring the declaration of the value as close to its
use as possible (in this case, reducing that distance to zero).
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> @@ -111,12 +110,11 @@ public void fileteredEmptyListShouldBeAnEmptyList() {
@test
@ignore
public void shouldFilterNonEmptyList() {
- final List<Integer> expected
- = Collections.unmodifiableList(Arrays.asList(1, 3));
final List<Integer> list = Collections.unmodifiableList(
🤔 I think it is really important that we take the opportunity to
show-off the Java Streams API as much as we can, here; using the
IntStream.range() idiom being one of those. But in this specific case, as
written, it feels a little awkward to use Arrays.asList() for a
two-element list and then upshift to IntStream to create a four-element
list.
💡 you *could* create a large input list and use a highly-selective
function in filter() that yields a short list. For example, a list from
[1, 21) where values are evenly divisible by 5?
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> @@ -343,9 +325,8 @@ public void shouldConcatenateSeveralLists() {
sortedCancatenatedList.addAll(list3);
sortedCancatenatedList.addAll(list4);
Collections.sort(sortedCancatenatedList);
❓ I'm confused as to what's motivating the sort. I was expecting to see a
few chunks that are, in series, appended. In fact, I'm expecting that the
order is preserved. Makes me feel like I'm missing a subtle point.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIezhyqKjyCpMOUqyN65nR1Qhc2mc8vmks5rTjwAgaJpZM4LHxsk>
.
|
|
Agreed, Removed this.
…On Mon, Jan 16, 2017 at 11:59 AM, John Ryan ***@***.***> wrote:
@javaeeeee <https://github.com/javaeeeee> said:
As to the append() method, it's not difficult to remove it from the
exercise. An idea why it could stay here is that while it can be bad for
production code for the reasons you mentioned, but here, in an exercise, it
could prod a learner to write some additional lines of code. Anyway, your
decision is needed here about what to do with this code.
Well, I've convinced myself that my idea is great! 😆
No, seriously, if you're actually on the fence, then my vote is to remove
append(). *However*, if you see additional benefit to the practitioner,
I'm super open to that.
I'm evaluating the additional lines of code that append() would provoke
and asking, "is there an additional learning experience stimulated by this
requirement?" The learnings I'm hoping for are: a language feature,
interesting design quandaries, showing-off the benefit of well-factored
code... I'm sure there's other categories. Do you see added value here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIezh17FeQujEoJhIPR7OHrp4pzOGGYUks5rS6H1gaJpZM4LHxsk>
.
|
|
It's okay. I am completely comfortable with the pace we move forward.
…On Mon, Jan 16, 2017 at 12:05 PM, John Ryan ***@***.***> wrote:
Hey @javaeeeee <https://github.com/javaeeeee> I meant to get to your PR,
this morning. The work day is getting started, so I'm gonna have to kick it
to tomorrow. Sorry for the delay.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIezh3uxvm-SFGQJw-bkIuweAVs3v-LVks5rS6NkgaJpZM4LHxsk>
.
|
|
Your ideas are definitely reasonable and benefit users. Also, I am
participating just for pleasure, so making changes is not a problem.
…On Wed, Jan 18, 2017 at 11:21 AM, John Ryan ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Again, sorry for the delay, Dmitry.
We're definitely making progress!
I seem to be really harping on making test code as simple as possible. In
particular, setting up expected values as literally as possible.
I'm painfully aware that my feedback is rather pedantic and particularly
opinionated. I hope that I'm doing a good job of motivating these changes.
At the heart of it a desire to create a delightful experience for our
users. Not unlike in literature, refinement is often about seeking concise
ways of expressing an idea.
It's a kind of empathy to put ourselves in the shoes of someone learning
Java (and/or seeing these language features for the first time) and asking
ourselves how much effort it would take to understand the
problem/question/test.
Empathy skillfully conveyed lands as respect. It's that we took the time
to minimize how much translation/interpretation we ask of our users to
understand the question/problem we're posing. They may not consciously know
it, but they'll feel the difference.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> @@ -53,23 +53,22 @@ public void shouldReverseAnEmptyList() {
@test
@ignore
- public void shouldReverseNonEmptyList() {
+ public void shouldReverseANonEmptyList() {
final int length = 100;
👍 extracting a value to a variable is a great move where the input and
output need to agree.
🤔 this larger number forces us to use IntStream.range() to generate the
list (and applying an operation on top of it; here sorting). If we start
with a smaller value, we could *start* with the transformed value.
Example:
final List<Integer> list = Arrays.asList(0, 1, 2, 3, 4, 5, 6, 7, 8);final List<Integer> reversed = Arrays.asList(8, 7, 6, 5, 4, 3, 2, 1, 0);
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> final List<Integer> list = Collections.unmodifiableList(
IntStream.range(startValue, length)
.boxed()
.collect(Collectors.toList())
);
final List<Integer> actual
= ListOps.reverse(list);
+ final List<Integer> expected = reversedList;
🤔 what do you think of using reversedList *as* the expected value
directly in the assert? I'm thinking this could increase the readability of
the assert itself.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> final List<Integer> list
= Collections.unmodifiableList(Arrays.asList(1, 3, 5, 7));
final List<Integer> actual = ListOps.map(list, x -> x + 1);
+ final List<Integer> expected
🤔 (redundant suggestion) this "literal" (as close as we get in Java) is
short enough that it *could* fit inline in the assert, below. The
motivation, here, is to bring the declaration of the value as close to its
use as possible (in this case, reducing that distance to zero).
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> @@ -111,12 +110,11 @@ public void fileteredEmptyListShouldBeAnEmptyList() {
@test
@ignore
public void shouldFilterNonEmptyList() {
- final List<Integer> expected
- = Collections.unmodifiableList(Arrays.asList(1, 3));
final List<Integer> list = Collections.unmodifiableList(
🤔 I think it is really important that we take the opportunity to
show-off the Java Streams API as much as we can, here; using the
IntStream.range() idiom being one of those. But in this specific case, as
written, it feels a little awkward to use Arrays.asList() for a
two-element list and then upshift to IntStream to create a four-element
list.
💡 you *could* create a large input list and use a highly-selective
function in filter() that yields a short list. For example, a list from
[1, 21) where values are evenly divisible by 5?
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> @@ -343,9 +325,8 @@ public void shouldConcatenateSeveralLists() {
sortedCancatenatedList.addAll(list3);
sortedCancatenatedList.addAll(list4);
Collections.sort(sortedCancatenatedList);
❓ I'm confused as to what's motivating the sort. I was expecting to see a
few chunks that are, in series, appended. In fact, I'm expecting that the
order is preserved. Makes me feel like I'm missing a subtle point.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIezhyqKjyCpMOUqyN65nR1Qhc2mc8vmks5rTjwAgaJpZM4LHxsk>
.
|
|
Thank you, @jtigger, for thorough review and advice. 👍 |
| @Ignore | ||
| public void shouldReturnTheCorrectLengthOfAnNonEmptyList() { | ||
| final List<Integer> list = Collections.unmodifiableList( | ||
| IntStream.range(0, 4) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
| public void shouldReverseANonEmptyList() { | ||
| final List<Integer> list = Collections.unmodifiableList( | ||
| Arrays.asList(0, 1, 2, 3, 4, 5, 6, 7, 8) | ||
| ); |
| public void shouldConcatenateOneNonEmptyList() { | ||
| final List<Integer> list | ||
| = Collections.unmodifiableList( | ||
| IntStream.range(0, 4) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
| ); | ||
| final List<Integer> actual = ListOps.concat(list); | ||
| final List<Integer> expected | ||
| = IntStream.range(0, 4) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
| public void shouldConcatenateOneEmptyAndOneNonEmptyLists() { | ||
| final List<Integer> list | ||
| = Collections.unmodifiableList( | ||
| IntStream.range(0, 4) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
| accumulator, | ||
| combiner); | ||
| final List<Integer> expected | ||
| = IntStream.range(0, 5) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
| @Ignore | ||
| public void shouldReduceTwoNonEmptyListsAndReturnConcatenation() { | ||
| final List<Integer> listOne = Collections.unmodifiableList( | ||
| IntStream.range(0, 5) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
| .collect(Collectors.toList()) | ||
| ); | ||
| final List<Integer> listTwo = Collections.unmodifiableList( | ||
| IntStream.range(5, 10) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
| accumulator, | ||
| combiner); | ||
| final List<Integer> expected | ||
| = IntStream.range(0, 10) |
There was a problem hiding this comment.
🔧 use Arrays.asList(...) for short lists.
|
|
||
| @Test | ||
| @Ignore | ||
| public void shouldReduceWithAnticommutativeAccumulator() { |
There was a problem hiding this comment.
🤔 wow... okay, I'm conflicted.
On the one hand, I love the notion of really taking it deep in this exercise. This test does that — it's an invitation to really dig into the Java Streams API.
On the other hand, it's really deep for someone just getting started. This might mean we're throwing a bone to those who are already familiar with the API and haven't dug deeply into this more complete form of reduce. Most of what I see in the wild sticks with collect(), but that's missing out. It might also be too big of a jump in this one exercise...
👥 @exercism/java: can I get another opinion on this?
There was a problem hiding this comment.
@jtigger Hello, John, added some fun with collect(). :)
|
*@jtigger* Hello, John,
Sorry for the delay. Thank you for your comments. It looks great to replace
ranges with Arrays.asList(). I'll do it soon.
"what if we made it feasible to express expected literally? For example, if
we made each listN shorter, the concatenated list would fit on one line."
Do you mean that we shouldn't compute expected, but just provide the range
for it or an explicit result in some other form?
"On the other hand, it's *really* deep for someone just getting started."
Thought about levels of difficulty including previous discussions about
Generics. We could provide two test files, one for those 'more comfortable'
and the one for 'less comfortable' as done in cs50x. So, we'll offer an
opportunity for beginners to start with simple things and an opportunity
for those who are preparing for an interview to master some advanced stuff.
The exercism site allows one to upload several solutions.
…On Sun, Jan 22, 2017 at 5:48 PM, John Ryan ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Another round of comments. I'm calling out each instance of short lists
being generated with IntStream.
There's also a request for another maintainers to chime in on the question
about using the more "complete" version of reduce().
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + private static final List<Integer> EMPTY_LIST
+ = Collections.emptyList();
+
+ @test
+ public void lengthOfAnEmptyListShouldBeZero() {
+ final int expected = 0;
+ final int actual = ListOps.length(EMPTY_LIST);
+
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReturnTheCorrectLengthOfAnNonEmptyList() {
+ final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, 4)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ @test
+ @ignore
+ public void shouldReverseAnEmptyList() {
+ final List<Integer> actual = ListOps.reverse(EMPTY_LIST);
+
+ assertNotNull(actual);
+ assertTrue(actual.isEmpty());
+ }
+
+ @test
+ @ignore
+ public void shouldReverseANonEmptyList() {
+ final List<Integer> list = Collections.unmodifiableList(
+ Arrays.asList(0, 1, 2, 3, 4, 5, 6, 7, 8)
+ );
👍
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ @test
+ @ignore
+ public void shouldConcatenateZeroLists() {
+ List<Integer> actual = ListOps.concat();
+
+ assertNotNull(actual);
+ assertTrue(actual.isEmpty());
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateOneNonEmptyList() {
+ final List<Integer> list
+ = Collections.unmodifiableList(
+ IntStream.range(0, 4)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + assertNotNull(actual);
+ assertTrue(actual.isEmpty());
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateOneNonEmptyList() {
+ final List<Integer> list
+ = Collections.unmodifiableList(
+ IntStream.range(0, 4)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final List<Integer> actual = ListOps.concat(list);
+ final List<Integer> expected
+ = IntStream.range(0, 4)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ @test
+ @ignore
+ public void shouldConcatenateTwoEmptyLists() {
+ final List<Integer> actual = ListOps.concat(EMPTY_LIST, EMPTY_LIST);
+
+ assertNotNull(actual);
+ assertTrue(actual.isEmpty());
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateOneEmptyAndOneNonEmptyLists() {
+ final List<Integer> list
+ = Collections.unmodifiableList(
+ IntStream.range(0, 4)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + assertNotNull(actual);
+ assertTrue(actual.isEmpty());
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateOneEmptyAndOneNonEmptyLists() {
+ final List<Integer> list
+ = Collections.unmodifiableList(
+ IntStream.range(0, 4)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final List<Integer> actual = ListOps.concat(list, EMPTY_LIST);
+ final List<Integer> expected
+ = IntStream.range(0, 4)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + final List<Integer> expected
+ = IntStream.range(0, 4)
+ .boxed()
+ .collect(Collectors.toList());
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateOneNonEmptyAndOneEmptyLists() {
+ final List<Integer> list
+ = Collections.unmodifiableList(
+ IntStream.range(0, 4)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateOneNonEmptyAndOneEmptyLists() {
+ final List<Integer> list
+ = Collections.unmodifiableList(
+ IntStream.range(0, 4)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final List<Integer> actual = ListOps.concat(EMPTY_LIST, list);
+ final List<Integer> expected
+ = IntStream.range(0, 4)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + final List<Integer> actual = ListOps.concat(EMPTY_LIST, list);
+ final List<Integer> expected
+ = IntStream.range(0, 4)
+ .boxed()
+ .collect(Collectors.toList());
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateTwoListsWithSameElements() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + .boxed()
+ .collect(Collectors.toList());
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateTwoListsWithSameElements() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(1, 6).boxed().collect(Collectors.toList())
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ assertNotNull(actual);
+ assertTrue(actual.isEmpty());
+ }
+
+ @test
+ @ignore
+ public void shouldFilterNonEmptyList() {
+ Predicate<Integer> predicate = x -> x % 2 > 0;
+ final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, 100).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> actual = ListOps.filter(list, predicate);
+ final List<Integer> expected = list.stream()
+ .filter(predicate)
+ .collect(Collectors.toList());
👍 I think I had suggested in a previous review that you eliminate the
need to calculate expected by making the expected result much smaller
(and therefore easy to express literally). *However*, I see now that this
is a great opportunity to highlight the Java Stream API. I dig it.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateTwoListsWithSameElements() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(1, 6).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> expected
+ = Stream.concat(list1.stream(), list2.stream())
+ .collect(Collectors.toList());
👍 ditto comment from above about putting the Java Streams API right next
to the ListOps operation of the same name to introduce it. In fact, this
one even more so since we're talking about concatenating Streams instead
of Lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(4, 8).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list3 = Collections.unmodifiableList(
+ IntStream.range(8, 12).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list4 = Collections.unmodifiableList(
+ IntStream.range(12, 16).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> expected
+ = new ArrayList<>(list1);
+ expected.addAll(list2);
+ expected.addAll(list3);
+ expected.addAll(list4);
🤔 what if we made it feasible to express expected literally? For
example, if we made each listN shorter, the concatenated list would fit
on one line.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + );
+ final List<Integer> expected
+ = Stream.concat(list1.stream(), list2.stream())
+ .collect(Collectors.toList());
+ final List<Integer> actual = ListOps.concat(list1, list2);
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateSeveralLists() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + .collect(Collectors.toList());
+ final List<Integer> actual = ListOps.concat(list1, list2);
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateSeveralLists() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(4, 8).boxed().collect(Collectors.toList())
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldConcatenateSeveralLists() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(4, 8).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list3 = Collections.unmodifiableList(
+ IntStream.range(8, 12).boxed().collect(Collectors.toList())
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + }
+
+ @test
+ @ignore
+ public void shouldConcatenateSeveralLists() {
+ final List<Integer> list1 = Collections.unmodifiableList(
+ IntStream.range(0, 4).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list2 = Collections.unmodifiableList(
+ IntStream.range(4, 8).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list3 = Collections.unmodifiableList(
+ IntStream.range(8, 12).boxed().collect(Collectors.toList())
+ );
+ final List<Integer> list4 = Collections.unmodifiableList(
+ IntStream.range(12, 16).boxed().collect(Collectors.toList())
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ @test
+ @ignore
+ public void shouldReturnIdentityWhenAnEmptyListIsReduced() {
+ final int expected = 0;
+ final int actual
+ = ListOps.reduce(EMPTY_LIST, 0, (x, y) -> x + y, Integer::sum);
+
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldCalculateTheSumOfANonEmptyIntegerList() {
+ final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, 5)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + IntStream.range(0, 5)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final int actual = ListOps.reduce(list, 0,
+ (x, y) -> x + y,
+ Integer::sum);
+
+ assertEquals(10, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReduceWithAnticommutativeAccumulator() {
+ final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, 5)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + result.add(elem);
+ return result;
+ };
+
+ private BinaryOperator<List<Integer>> combiner
+ = (list1, list2) -> {
+ List<Integer> result = new ArrayList<>(list1);
+ result.addAll(list2);
+ return result;
+ };
+
+ @test
+ @ignore
+ public void shouldReduceAnEmptyListAndANonEmptyListAndReturnConcatenation() {
+ final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, 5)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ @test
+ @ignore
+ public void shouldReduceAnEmptyListAndANonEmptyListAndReturnConcatenation() {
+ final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, 5)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final List<Integer> actual
+ = ListOps.reduce(list,
+ new ArrayList<Integer>(),
+ accumulator,
+ combiner);
+ final List<Integer> expected
+ = IntStream.range(0, 5)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + combiner);
+ final List<Integer> expected
+ = IntStream.range(0, 5)
+ .boxed()
+ .collect(Collectors.toList());
+
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReduceTwoNonEmptyListsAndReturnConcatenation() {
+ final List<Integer> listOne = Collections.unmodifiableList(
+ IntStream.range(0, 5)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> +
+ assertNotNull(actual);
+ assertFalse(actual.isEmpty());
+ assertEquals(expected, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReduceTwoNonEmptyListsAndReturnConcatenation() {
+ final List<Integer> listOne = Collections.unmodifiableList(
+ IntStream.range(0, 5)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final List<Integer> listTwo = Collections.unmodifiableList(
+ IntStream.range(5, 10)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + IntStream.range(0, 5)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final List<Integer> listTwo = Collections.unmodifiableList(
+ IntStream.range(5, 10)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final List<Integer> actual
+ = ListOps.reduce(listTwo,
+ listOne,
+ accumulator,
+ combiner);
+ final List<Integer> expected
+ = IntStream.range(0, 10)
🔧 use Arrays.asList(...) for short lists.
------------------------------
In exercises/list-ops/src/test/java/ListOpsTest.java
<#207 (review)>:
> + public void shouldCalculateTheSumOfANonEmptyIntegerList() {
+ final List<Integer> list = Collections.unmodifiableList(
+ IntStream.range(0, 5)
+ .boxed()
+ .collect(Collectors.toList())
+ );
+ final int actual = ListOps.reduce(list, 0,
+ (x, y) -> x + y,
+ Integer::sum);
+
+ assertEquals(10, actual);
+ }
+
+ @test
+ @ignore
+ public void shouldReduceWithAnticommutativeAccumulator() {
🤔 wow... okay, I'm conflicted.
On the one hand, I love the notion of really taking it deep in this
exercise. This test does that — it's an invitation to really dig into the
Java Streams API.
On the other hand, it's *really* deep for someone just getting started.
This might mean we're throwing a bone to those who are already familiar
with the API and haven't dug deeply into this more complete form of reduce.
Most of what I see in the wild sticks with collect(), but that's missing
out. It might also be too big of a jump in this one exercise...
👥 @exercism/java <https://github.com/orgs/exercism/teams/java>: can I
get another opinion on this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AIezh1xlJ-0X8PlehRnvR4ELNNBIfnOyks5rU9zDgaJpZM4LHxsk>
.
|
I try to be really careful with the auxiliary verb "should" because I believe that decisions are contextual. If we take that word as a shorthand for "what seems appropriate in this case", then we're talking the same language. :) I do think we "should" aim to craft examples that take a little thought-power as possible to read. If the target outcome is literal, that's often the easiest to read. Otherwise, I'm asking the reader to envision the expectation, rather than just see it.
Yeah, man! @matthewmorgan and I have had a number of conversations along these lines. I could have sworn we created an issue in discussion or x-common around this, but I can't seem to find it. At the time, the idea would be that we specifically add "bonus" requirements sections in the common description; some would simply add to the depth of the exercise, some would often cause the practitioner to likely seriously restructure their solution. But this seems like a track-specific version... hadn't occurred to me, before that we could just add this to the I'd be totally down for that. |
There was a problem hiding this comment.
At this point, this PR has been very long running; I'm eager to land this plane.
Highlights:
- The diff reveals a number of places where there are trailing spaces. Please trim these.
- There still remain (at least one) place(s) where actuals are calculated where they can be expressed literally. Please don't wait for me to point out each instance, but proactively comb for such instances and address them. Thanks!
- I question the wisdom of increasing the scope of this PR. I'm starting to experience reviewer fatigue, personally.
Details in individual comments.
Thanks for your efforts!
| ); | ||
| final List<Integer> list2 = Collections.unmodifiableList( | ||
| IntStream.range(1, 6).boxed().collect(Collectors.toList()) | ||
| Arrays.asList(1, 2, 3, 4, 5, 6) | ||
| ); | ||
| final List<Integer> expected | ||
| = Stream.concat(list1.stream(), list2.stream()) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
🔧 expected can be feasibly expressed literally, here.
| assertEquals(10, actual); | ||
| } | ||
|
|
||
| @Test | ||
| @Ignore | ||
| public void shouldReduceWithAnticommutativeAccumulator() { |
There was a problem hiding this comment.
🤔 coming back to this topic with a "fresh" mind... mentioning "parallelization" without a requirement to support parallelization in the exercise is a likely source of confusion. That's a problem.
I see two paths:
- add the requirement to be able to parallelize stream operations.
- remove the notion of parallelization from this exercise (and attendant APIs that are required by it).
We've packed so much into this exercise, as it is, I'm personally leaning toward option 2.
@javaeeeee, do you see another option? What can we do to avoid this confusion?
|
|
||
| @Test | ||
| @Ignore | ||
| public void shouldReturnZeroWhenAnEmptyListIsCollected() { |
There was a problem hiding this comment.
😱 OH GEEEZ! More scope?!?! :)
This is already the longest running PR I've seen on this track. I strongly prefer we first land this thing before adding any more to the exercise.
It opens a can of worms as to whether to include both reduce and collect; the two are very similar. And it duplicates the concern of invoking the notion of parallelizable processing of stream operations.
Would you mind stashing this work somewhere? I suspect we'll break this thing up into chunks and it won't seem so overwhelming... later.
|
Hey, @javaeeeee, we doing okay, here? I'm not trying to rush you, just want to make sure we're still together in this. |
|
@jtigger Hello, John, sorry for the delay. Was unable to make changes
because being swamped with work. We are definitely together. Will finish it
ASAP.
…On Fri, Feb 10, 2017 at 9:20 AM, John Ryan ***@***.***> wrote:
Hey, @javaeeeee <https://github.com/javaeeeee>, we doing okay, here? I'm
not trying to rush you, just want to make sure we're still together in this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIezh0c5gXE5zb-6FwQPhwZvfoO8U61-ks5rbHJIgaJpZM4LHxsk>
.
|
|
No rush, @javaeeeee. I had a lot of constructive criticism in my last round of review. I wanted to make sure I didn't hit a nerve. :) Please take your time, Dimitry. We're not in a rush here. This should be the fun thing you do in your spare time — not another source of "work." |
|
Everything is okay. Just need a time window. :)
…On Fri, Feb 10, 2017 at 9:28 AM, John Ryan ***@***.***> wrote:
No rush, @javaeeeee <https://github.com/javaeeeee>. I had a lot of
constructive criticism in my last round of review. I wanted to make sure I
didn't hit a nerve. :)
Please take your time, Dimitry. We're not in a rush here. This should be
the *fun* thing you do in your spare time — not another source of "work."
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIezh_drHnRbxm147QP9etfRXFhrmlr3ks5rbHP6gaJpZM4LHxsk>
.
|
|
@jtigger Hello, John, Sorry once again for the delay.
|
|
Merged with master to resolve conflicts in |
|
By the power invested in me, Dmitry, I do declare your PR as one fantastic piece of work. Thank you for your patience and stick-to-it-ness. :) This exercise is a great add to the track. |
|
@jtigger Thank you, John :)
…On Mon, Feb 20, 2017 at 10:20 PM, John Ryan ***@***.***> wrote:
Merged #207 <#207>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#207 (comment)>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AIezh68X-eNt8RX5bUpR2tI9OqdoUTVmks5relgOgaJpZM4LHxsk>
.
|
dibs: I will implement list-ops