Added solution for SalaryInfo class - #1480
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
REQUEST_CHANGES
Thank you for your submission! Your code is clear and the salary calculation logic is correct, but there are two critical issues that must be fixed to meet the assignment's explicit requirements:
-
Use of Map/HashMap for Salary Aggregation
- The requirements state: "Do not use HashMap or any other Map." Your current implementation uses a
Mapto store and sum salaries, which is not allowed. - How to fix: Replace the
Mapwith anint[](or similar array) where each index corresponds to an employee in thenamesarray. Aggregate salaries using this array instead.
- The requirements state: "Do not use HashMap or any other Map." Your current implementation uses a
-
DateTimeFormatter Must Be a Constant Field
- The requirements specify: "If a date formatter is created, it must be a constant field." Currently, your
DateTimeFormatteris an instance field. - How to fix: Move your formatter to a
private static finalfield, for example:private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("dd.MM.yyyy");
- The requirements specify: "If a date formatter is created, it must be a constant field." Currently, your
Once you address these two issues, your code should fully comply with the assignment's requirements. The rest of your implementation is well-structured and readable—great job on that! If you have any questions about how to refactor the aggregation logic, feel free to ask. Keep going, you're almost there!
✨ 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 null; | ||
| LocalDate from = LocalDate.parse(dateFrom, dateTimeFormatter); | ||
| LocalDate to = LocalDate.parse(dateTo, dateTimeFormatter); | ||
| Map<String, Integer> salaryMap = new HashMap<>(); |
There was a problem hiding this comment.
This violates the requirement: "Do not use HashMap or any other Map." You must use arrays for salary aggregation instead of a Map.
| import java.util.Map; | ||
|
|
||
| public class SalaryInfo { | ||
| private final DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
This violates the requirement: "If a date formatter is created, it must be a constant field." The formatter should be a static final field, not an instance field.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your solid implementation—the core logic and all major requirements are met! However, there are a few explicit checklist violations that must be addressed before approval:
-
Magic Numbers as Constants
- The indices used when splitting the data line (e.g.,
parts[0],parts[1], etc.) are hardcoded. The checklist requires all such magic numbers to be declared as constants with descriptive names (e.g.,INDEX_DATE,INDEX_NAME). - This improves readability and maintainability, and is a required standard for this assignment.
- The indices used when splitting the data line (e.g.,
-
DateTimeFormatter Constant Naming
- The date formatter constant should be named in uppercase with underscores (e.g.,
DATE_TIME_FORMATTER) according to Java conventions and the checklist.
- The date formatter constant should be named in uppercase with underscores (e.g.,
-
Redundant Empty Lines
- There are unnecessary empty lines at the start of the file and after the class declaration. The checklist specifies that redundant empty lines must be avoided.
How to fix:
- Declare constants for all array indices used when splitting data lines.
- Rename your date formatter constant to
DATE_TIME_FORMATTER. - Remove any unnecessary empty lines at the start of the file and after the class declaration.
Once you address these points, your submission will fully comply with all requirements. Great job on the core logic—just a few style and checklist details to polish up!
✨ 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
| int[] salaries = new int[names.length]; | ||
|
|
||
| for (String record : data) { | ||
| String[] parts = record.split(" "); |
There was a problem hiding this comment.
This violates the checklist item: "All hardcoded values (magic numbers) must be moved to constants with descriptive names." The indices used for splitting the data line (e.g., parts[0], parts[1], parts[2], parts[3]) should be declared as constants.
| public class SalaryInfo { | ||
| public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
| private static final DateTimeFormatter dateTimeFormatter = |
There was a problem hiding this comment.
This violates the checklist item: "If a date formatter is used, it must be a constant with an informative name." The constant should be named in uppercase with underscores (e.g., DATE_TIME_FORMATTER) as per Java conventions and the checklist.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to help you continue progressing in your learning journey. Your implementation of the SalaryInfo class is functionally correct and demonstrates strong understanding of Java fundamentals and assignment requirements.
What you did well:
- Used
LocalDateand a constantDateTimeFormatterfor date handling, which is best practice. - Declared all array indices as descriptive constants, improving readability and maintainability.
- Followed Java naming conventions and kept your code clean and well-organized.
- Ensured all employees from the
namesarray are included in the output, even with zero salary. - Avoided forbidden structures like
Mapor 2D arrays, and used only two loops as required.
Areas for future improvement:
- Always double-check for any redundant code or empty lines, especially at the start of files or after class declarations.
- Continue to use descriptive variable and constant names to make your code easy to understand.
- Think about edge cases, such as handling unexpected input formats or empty data arrays, to make your code even more robust.
Keep up the great work! Your attention to detail and willingness to address feedback are excellent habits. With continued practice, you'll further strengthen your coding skills and confidence.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
Added solution for SalaryInfo class