first-attempt#1412
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This submission has several critical issues that must be addressed. First, getOddNumsAverage() uses a for-loop to modify the list before streaming, which violates the Stream API requirement from the checklist. Second, getWorkablePeople() has an operator precedence bug in the second filter — the age upper bound check isn't properly combined with the category filter, causing incorrect filtering results. Third, getCatsNames() uses > instead of >= for the femaleAge comparison, excluding women who are exactly at the boundary when the requirement specifies "inclusively." Additionally, all enum comparisons use == instead of .equals(), and CandidateValidator contains magic numbers (35, 10) and strings ("Ukrainian") instead of declared constants — both are checklist violations. Please fix these functional bugs and checklist violations before resubmission.
✨ 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
| .filter(n -> n % 2 == 0) | ||
| .min() | ||
| .orElseThrow(() -> | ||
| new RuntimeException("Can`t get min value from list: " + numbers)); |
There was a problem hiding this comment.
Backtick character used instead of single quote in exception message. Should be "Can't" not "Can`t"
| if (numbers == null || numbers.isEmpty()) { | ||
| throw new NoSuchElementException("List is empty or null from list: " + numbers); | ||
| } | ||
| for (int i = 1; i < numbers.size(); i += 2) { |
There was a problem hiding this comment.
CHECKLIST VIOLATION: Uses a for-loop to modify list elements. Task requires using Stream API exclusively, no loops.
| return Collections.emptyList(); | ||
| } | ||
| return peopleList.stream() | ||
| .filter(p -> p.getSex() == Person.Sex.MAN) |
There was a problem hiding this comment.
CHECKLIST VIOLATION: Enum values should be compared using .equals() method, not == operator.
| } | ||
| return peopleList.stream() | ||
| .filter(p -> p.getAge() >= fromAge) | ||
| .filter(p -> p.getAge() <= maleToAge && p.getSex() == Person.Sex.MAN |
There was a problem hiding this comment.
CHECKLIST VIOLATION: Enum values should be compared using .equals() method, not == operator.
| return Collections.emptyList(); | ||
| } | ||
| return peopleList.stream() | ||
| .filter(p -> p.getAge() > femaleAge && p.getSex() == Person.Sex.WOMAN) |
There was a problem hiding this comment.
CHECKLIST VIOLATION: Enum values should be compared using .equals() method, not == operator.
| return Collections.emptyList(); | ||
| } | ||
| return peopleList.stream() | ||
| .filter(p -> p.getAge() > femaleAge && p.getSex() == Person.Sex.WOMAN) |
There was a problem hiding this comment.
Should be p.getAge() >= femaleAge (use >= for inclusive boundary as per requirements: 'from femaleAge years old inclusively').
| import java.util.NoSuchElementException; | ||
| import java.util.stream.Collectors; | ||
| import model.Candidate; | ||
| import model.Cat; |
There was a problem hiding this comment.
CHECKLIST VIOLATION: Magic number '35' should be extracted to a constant (e.g., MIN_AGE).
| import java.util.stream.Collectors; | ||
| import model.Candidate; | ||
| import model.Cat; | ||
| import model.Person; |
There was a problem hiding this comment.
CHECKLIST VIOLATION: Magic string 'Ukrainian' should be extracted to a constant.
| @Override | ||
| public boolean test(Candidate candidate) { | ||
| if (candidate.getAge() < 35 || !candidate.isAllowedToVote() | ||
| || !"Ukrainian".equals(candidate.getNationality())) { |
There was a problem hiding this comment.
Magic string 'Ukrainian' should be a constant. Checklist requires using constants in CandidateValidator to avoid magic numbers/strings.
| public class CandidateValidator implements Predicate<Candidate> { | ||
| @Override | ||
| public boolean test(Candidate candidate) { | ||
| if (candidate.getAge() < 35 || !candidate.isAllowedToVote() |
There was a problem hiding this comment.
Magic number 35 should be a constant. Checklist requires using constants in CandidateValidator to avoid magic numbers/strings.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your code has one critical functional bug that must be fixed. In getWorkablePeople(), the operator precedence in the second filter is incorrect: p.getAge() <= maleToAge && Person.Sex.MAN.equals(p.getSex()) binds tighter than ||, causing the method to return wrong results. You need to wrap each gender condition in parentheses like: .filter(p -> p.getAge() >= fromAge && ((p.getSex().equals(Person.Sex.MAN) && p.getAge() <= maleToAge) || (p.getSex().equals(Person.Sex.WOMAN) && p.getAge() <= femaleToAge))). There's also a minor checklist violation in selectMenByAge() where p.getSex() == Person.Sex.MAN should use .equals() per checklist item #5. The enum comparison works correctly with == in Java, but should still be fixed for consistency.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override to help you progress while learning. Great job fixing the selectMenByAge() method to use .equals() for enum comparison instead of == - that's an important distinction in Java. Your implementation handles most requirements correctly and shows solid understanding of Stream API usage. However, there's one remaining issue in getWorkablePeople() at lines 86-87: the filter condition needs explicit parentheses around each gender group. Currently it relies on Java's operator precedence where && binds tighter than ||, but wrapping them like ((sex == MAN && age <= maleToAge) || (sex == WOMAN && age <= femaleToAge)) makes the logic clearer and less error-prone. This is a best practice for boolean expressions mixing && and ||. Keep up the good work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| .filter(p -> p.getAge() <= maleToAge && p.getSex().equals(Person.Sex.MAN) | ||
| || p.getAge() <= femaleToAge && p.getSex().equals(Person.Sex.WOMAN)) |
There was a problem hiding this comment.
This filter has incorrect operator precedence. Due to && binding tighter than ||, the expression groups as (age <= maleToAge && sex == MAN) || (age <= femaleToAge && sex == WOMAN). While it works currently because of the preceding filter, wrap each gender condition in parentheses for correctness: ((p.getSex().equals(Person.Sex.MAN) && p.getAge() <= maleToAge) || (p.getSex().equals(Person.Sex.WOMAN) && p.getAge() <= femaleToAge))
2 participants
No description provided.