homeSo;#1395
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a solid attempt, and you've correctly used the Stream API for most of the tasks. However, some changes are needed in the CandidateValidator class as its current implementation doesn't meet the requirements, which causes the candidate selection task to fail.
Specifically, there are two issues that need to be addressed:
- Incorrect Age Validation: The requirement is for a candidate to be "at least 35 years old" (inclusive). Your current logic incorrectly rejects candidates who are exactly 35. Please adjust the condition to correctly include them.
- Magic String Usage: The hardcoded string
"Ukrainian"violates the project's coding standards [CHECKLIST ITEM #6]. This value should be declared as aprivate static final Stringconstant at the top of the class.
Once you've fixed these two points, your solution will be ready. Keep up the great work!
✨ 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) { | ||
| if (c.getAge() <= MIN_AGE) { |
There was a problem hiding this comment.
The requirement is for a candidate to be "at least 35 years old", which is inclusive. This condition c.getAge() <= 35 incorrectly rejects candidates who are exactly 35. The check should ensure that ages 35 and older are considered valid.
| if (!c.isAllowedToVote()) { | ||
| return false; | ||
| } | ||
| if (!"Ukrainian".equals(c.getNationality())) { |
There was a problem hiding this comment.
This violates checklist item #6: 'Use constants'. The string "Ukrainian" is a magic value. It should be defined as a private static final String constant at the top of the class to improve readability and make future changes easier.
| public List<String> validateCandidates(List<Candidate> candidates) { | ||
| return Collections.emptyList(); | ||
| return candidates.stream() | ||
| .filter(new CandidateValidator()) |
There was a problem hiding this comment.
This filter uses CandidateValidator, which doesn't correctly implement the requirements.
- The age validation is for "older than 35" (
> 35), but thedescriptionfile requires "at least 35 years old" (>= 35). This will incorrectly exclude candidates who are exactly 35. - The validator uses the magic string
"Ukrainian". According to checklist item Stream practice by ok #6, this should be a 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 revision! You've successfully addressed all the previous feedback, and your solution now correctly implements all the required functionality. Your use of the Stream API is clean and efficient. I've left one minor inline comment in StreamPractice.java with a suggestion to improve readability, but it's not a required change. Please be sure to fix the Checkstyle violations reported by the automated checks so your pull request can be merged. Since all functional requirements are met, I am approving your submission. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| .filter(el -> el.getSex().equals(Person.Sex.MAN) | ||
| && el.getAge() >= fromAge | ||
| && el.getAge() <= maleToAge | ||
| || el.getSex().equals(Person.Sex.WOMAN) | ||
| && el.getAge() >= fromAge | ||
| && el.getAge() <= femaleToAge) |
There was a problem hiding this comment.
This logic is correct, but the combined condition is a bit long and can be hard to read. You can make it clearer by factoring out the common age check (el.getAge() >= fromAge). For example: .filter(el -> el.getAge() >= fromAge && ((isMan && age <= maleToAge) || (isWoman && age <= femaleToAge))).
2 participants
No description provided.