First Update#1393
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the overall implementation! The use of the Stream API is correct and all functional requirements have been met. However, I am requesting a change in the CandidateValidator.java file. The code currently uses hardcoded values (e.g., 35, 10, and "Ukrainian"), which are often called 'magic values'. This is a violation of a specific project requirement to use constants [CHECKLIST ITEM #6]. Please refactor this by declaring these values as named private static final constants. This will significantly improve the readability and maintainability of your code.
✨ 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 the usage of CandidateValidator here is correct, its implementation violates checklist item #6: 'Use constants'. The test method in the CandidateValidator class uses magic numbers (e.g., 35, 10) and a magic string ("Ukrainian"). These should be defined as constants for better readability and maintainability.
| return candidate.getAge() >= 35 | ||
| && candidate.isAllowedToVote() | ||
| && "Ukrainian".equals(candidate.getNationality()) | ||
| && yearsInUkr >= 10; |
There was a problem hiding this comment.
This violates checklist item #6: 'Use constants'. The numbers 35 and 10, and the string "Ukrainian" are 'magic values'. They should be declared as named constants (e.g., private static final int MIN_AGE = 35;) to make the code clearer and easier to maintain.
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 feedback from the last review! The CandidateValidator now correctly uses constants, which is a significant improvement. Your use of the Stream API is very well done.
I am requesting one small change in the getCatsNames method within src/main/java/practice/StreamPractice.java. The task description asks for the names of all cats that meet the criteria, but the use of .distinct() filters out duplicate names. For example, if two different owners have cats with the same name, that name should appear twice in the final list.
Please remove the .distinct() operation to align the implementation with the requirement. After this minor adjustment, 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
| .filter(p -> p.getAge() >= femaleAge) | ||
| .flatMap(p -> p.getCats().stream()) | ||
| .map(Cat::getName) | ||
| .distinct() |
There was a problem hiding this comment.
The task description asks for the names of all cats that meet the criteria. Using distinct() filters out duplicate names. If two different owners have cats with the same name, that name should appear twice in the final list, but this implementation would only return it once. Please consider removing this step to align with the requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on this task! Your implementation is clean, correct, and effectively uses the Stream API, especially with the custom CandidateValidator. I am approving your solution so you can continue making progress.
As a small tip for future improvements, you can make your code even more concise and efficient by combining consecutive filter() calls into a single one. For example, in selectMenByAge, you can merge the two filters using a logical &&. This is a good practice that helps minimize the number of operations on the stream.
.filter(p -> p.getSex() == Person.Sex.MAN
&& p.getAge() >= fromAge && p.getAge() <= toAge)This is just a minor refinement in an otherwise excellent submission. Well done!
✨ 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.
This is functionally correct, but you can make it more concise. It's a good practice to combine consecutive filter operations into a single one to reduce the number of operations on the stream. This aligns with checklist item #1.
You can combine these two filters like this:
.filter(p -> p.getSex() == Person.Sex.MAN
&& p.getAge() >= fromAge && p.getAge() <= toAge)| .filter(p -> p.getSex() == Person.Sex.WOMAN) | ||
| .filter(p -> p.getAge() >= femaleAge) |
There was a problem hiding this comment.
Great job addressing the feedback from the last review by removing .distinct()!
Similar to my other comment, you can combine these two filter calls into a single one for better conciseness and performance, as suggested by checklist item #1:
.filter(p -> p.getSex() == Person.Sex.WOMAN && p.getAge() >= femaleAge)2 participants
No description provided.