Candidate validator created and stream practice assingments done#1390
Candidate validator created and stream practice assingments done#1390ReduxR wants to merge 3 commits into
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This requires a few changes before it can be approved. While you've made good use of the Stream API, there are a few critical issues that need to be addressed.
First, in StreamPractice.java, the findMinEvenNumber method will fail with a NumberFormatException on inputs like "0, 22, 7" because the strings aren't trimmed before parsing. Additionally, the exception message in that method does not match the requirements from the checklist [CHECKLIST ITEM #8].
Second, in CandidateValidator.java, you are using 'magic numbers' (35, 10) and a 'magic string' ("Ukrainian"). These must be extracted into named constants as required by the checklist [CHECKLIST ITEM #6].
Please address these points, and I'll be happy to take another look!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| return 0; | ||
| return numbers.stream() | ||
| .flatMap(n -> Arrays.stream(n.split(","))) | ||
| .map(Integer::valueOf) |
There was a problem hiding this comment.
This mapping will throw a NumberFormatException if the number strings contain leading or trailing spaces, which is the case in the example ("0, 22, 7"). You should trim the strings before parsing them to integers.
| .map(Integer::valueOf) | ||
| .filter(num -> num % 2 == 0) | ||
| .min(Integer::compareTo) | ||
| .orElseThrow(() -> new RuntimeException("Can't get min value from list.")); |
There was a problem hiding this comment.
The exception message does not match the requirements. This violates checklist item #8, which specifies the message should be: "Can't get min value from list: " + numbers.
| return Collections.emptyList(); | ||
| return peopleList.stream() | ||
| .filter(person -> person.getAge() >= fromAge && person.getAge() <= toAge | ||
| && person.getSex().equals(Person.Sex.MAN)) |
There was a problem hiding this comment.
While .equals() works for enums, the task checklist suggests that comparing them with == is generally better for performance and it's null-safe. For example: person.getSex() == Person.Sex.MAN.
| int maleToAge, List<Person> peopleList) { | ||
| return Collections.emptyList(); | ||
| return peopleList.stream() | ||
| .filter(person -> person.getSex().equals(Person.Sex.MAN) |
There was a problem hiding this comment.
As mentioned in the checklist, using == is often a better way to compare enum values.
| public List<String> getCatsNames(List<Person> peopleList, int femaleAge) { | ||
| return Collections.emptyList(); | ||
| return peopleList.stream() | ||
| .filter(person -> person.getSex().equals(Person.Sex.WOMAN) |
There was a problem hiding this comment.
As noted before, the project checklist recommends using == to compare enum values.
| if (candidate == null) { | ||
| return false; | ||
| } | ||
| if (candidate.getAge() < 35) { |
There was a problem hiding this comment.
This condition uses a "magic number" (35). According to checklist item #6, it's better to declare this value as a named constant (e.g., private static final int MIN_AGE = 35;) to improve code readability.
| if (!candidate.isAllowedToVote()) { | ||
| return false; | ||
| } | ||
| if (!"Ukrainian".equals(candidate.getNationality())) { |
There was a problem hiding this comment.
This condition uses a "magic string" ("Ukrainian"). This violates checklist item #6. This value should be defined as a private static final constant.
| .mapToInt(arr -> Integer.parseInt(arr[1]) - Integer.parseInt(arr[0])) | ||
| .sum(); | ||
|
|
||
| return yearsInUkraine >= 10; |
There was a problem hiding this comment.
The number 10 is also a magic number, which goes against checklist item #6. Please extract it into a named constant.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You have successfully addressed all the feedback from the previous review, and the code now meets all functional requirements. I am approving your solution.
I have one minor suggestion for improving code style. In StreamPractice.java, it's good practice to move the .orElseThrow() call to a new line. This aligns with the guideline to call each new method on a new line in a stream, which improves readability [CHECKLIST ITEM #5].
This is just a minor formatting point, and the overall solution is solid. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return IntStream.range(0, numbers.size()) | ||
| .map(i -> i % 2 != 0 ? numbers.get(i) - 1 : numbers.get(i)) | ||
| .filter(n -> n % 2 != 0) | ||
| .average().orElseThrow(NoSuchElementException::new); |
There was a problem hiding this comment.
This violates checklist item #5: 'Call each new method from a new line in the stream'. For better readability, it's good practice to move the .orElseThrow() call to a new line, just like you did in the findMinEvenNumber method.
…ollectors calls in StreamPractice
3 participants
No description provided.