stream practice#1391
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on using the Stream API for these tasks! Your implementation is very clean. However, a couple of changes are needed in CandidateValidator.java because the current logic doesn't fully meet the requirements.
First, the age validation is slightly off. The requirement is for candidates to be at least 35 years old, which means the check should be inclusive (>= 35), but the current code uses > 35.
Second, the hardcoded values 35, 10, and "Ukrainian" are considered 'magic values'. According to the checklist [CHECKLIST ITEM #6], these should be defined as private static final constants to make the code more readable and maintainable.
Please address these two points, and your solution will be perfect!
✨ 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
|
|
||
| @Override | ||
| public boolean test(Candidate c) { | ||
| return c.getAge() > 35 |
There was a problem hiding this comment.
The requirement states that a candidate should be at least 35 years old, which means the age check should be inclusive (>=). Also, this violates checklist item #6: 'Use constants'. The magic number 35 should be declared as a constant.
| public boolean test(Candidate c) { | ||
| return c.getAge() > 35 | ||
| && c.isAllowedToVote() | ||
| && "Ukrainian".equals(c.getNationality()) |
There was a problem hiding this comment.
This violates checklist item #6: 'Use constants'. The string "Ukrainian" is a magic string and should be defined as a constant to improve readability.
| return c.getAge() > 35 | ||
| && c.isAllowedToVote() | ||
| && "Ukrainian".equals(c.getNationality()) | ||
| && getYearsInUkraine(c.getPeriodsInUkr()) >= 10; |
There was a problem hiding this comment.
This violates checklist item #6: 'Use constants'. The magic number 10 should be declared as a constant.
| public List<String> validateCandidates(List<Candidate> candidates) { | ||
| return Collections.emptyList(); | ||
| return candidates.stream() | ||
| .filter(new CandidateValidator()) // используем наш Predicate |
There was a problem hiding this comment.
While using new CandidateValidator() here is correct, its implementation in the CandidateValidator.java file has a couple of issues that cause the validation to fail for some cases:
- The age check should be
>= 35to include 35-year-olds, as per the 'at least 35' requirement. - Checklist item Stream practice by ok #6 requires avoiding magic numbers and strings.
35,10, and"Ukrainian"should be defined as constants inCandidateValidator.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback! The age validation logic is now correct, and you've successfully converted the magic numbers into constants.
I'm requesting one final change to fully meet the requirements. The string "Ukrainian" in CandidateValidator.java is still hardcoded. According to the checklist, all magic strings and numbers must be replaced with constants to improve code maintainability [CHECKLIST ITEM #6].
Please define it as a private static final String constant and use that constant in your validation logic. After that small fix, this will be ready to go!
✨ 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
| public List<String> validateCandidates(List<Candidate> candidates) { | ||
| return Collections.emptyList(); | ||
| return candidates.stream() | ||
| .filter(new CandidateValidator()) |
There was a problem hiding this comment.
While CandidateValidator is used correctly here, its implementation still contains a hardcoded string "Ukrainian". According to checklist item #6, magic strings should also be constants. Please define it as a private static final constant, similar to how you handled MIN_AGE.
| public boolean test(Candidate c) { | ||
| return c.getAge() >= MIN_AGE | ||
| && c.isAllowedToVote() | ||
| && "Ukrainian".equals(c.getNationality()) |
There was a problem hiding this comment.
This is a "magic string". According to the checklist, all magic numbers and strings should be replaced with constants. Please define "Ukrainian" as a private static final constant, just as you did for MIN_AGE.
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 task, and great job fixing the hardcoded string from the last review! Your solution meets all the requirements.
I am approving this submission to help you keep moving forward, but I have one small suggestion for improvement. In StreamPractice.java, you can combine consecutive filter() operations into a single one for better readability and efficiency. For example, instead of two separate filters, you can use a single one with conditions joined by &&:
.filter(person -> person.getSex() == Person.Sex.MAN && person.getAge() >= 18)This is a common best practice when working with streams. Overall, fantastic job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| .filter(p -> p.getSex() == Person.Sex.MAN) | ||
| .filter(p -> p.getAge() >= fromAge && p.getAge() <= toAge) |
There was a problem hiding this comment.
While this works, you can combine these two filter operations into a single one for better readability and to use a minimum number of stream operations, as suggested in the checklist. You can chain the conditions using &&.
| .filter(p -> p.getSex() == Person.Sex.WOMAN) | ||
| .filter(p -> p.getAge() >= femaleAge) |
There was a problem hiding this comment.
Similar to the selectMenByAge method, you can combine these two filter conditions into a single operation to make the stream pipeline more concise. This adheres to the guideline of using a minimum number of stream operations.
2 participants
No description provided.