-
-
Notifications
You must be signed in to change notification settings - Fork 951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Java/Word Count] Add mentoring notes #1023
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but pinging @exercism/website-copy-java
} | ||
``` | ||
|
||
FP solution (Java 11): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider writing out FP as
FP solution (Java 11): | |
Functional programming (FP) solution (Java 11+): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think most (if not all) of this actually works starting in Java 8, but I could be mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmrunkle, I agree that most of this working in Java 8. But java.util.function.Predicate.not;
is Java 11... So what should I do? ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can add that as a helper with the comment "Use java.util.function.Predicate.not in Java 11+"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I had forgotten that they didn't add that until Java 11. My company uses Java 8, but uses the Guava method so I just did not really notice: https://google.github.io/guava/releases/23.0/api/docs/com/google/common/base/Predicates.html#not-com.google.common.base.Predicate-
} | ||
``` | ||
|
||
FP solution (Java 11): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think most (if not all) of this actually works starting in Java 8, but I could be mistaken.
if (aWord.isEmpty()) { | ||
continue; | ||
} | ||
Integer count = countMap.get(aWord); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map has a merge method that is probably better for this use case.
.map(this::stripQuotes) | ||
.map(String::toLowerCase) | ||
.map(word -> new AbstractMap.SimpleEntry<>(word, 1)) | ||
.collect(groupingBy(entry -> entry.getKey(), summingInt(entry -> entry.getValue()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the previous line and make this change:
.collect(groupingBy(entry -> entry.getKey(), summingInt(entry -> entry.getValue()))); | |
.collect(groupingBy(identity(), summingInt(word -> 1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally true!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmrunkle would you like to have another look at this? If not can you ping me here and we get this merged.
No description provided.