Solved salary info task - #1580
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before approval. Your implementation is functionally correct and passes the core calculation logic. However, there are three explicit checklist violations that must be fixed: 1. [CHECKLIST ITEM #5] Using HashMap (Map<String, Integer>) is prohibited. Replace it with a manual approach using parallel arrays or calculated indices. 2. [CHECKLIST ITEM #4] The code uses three loops (initialization, processing, result building) instead of the allowed maximum of two. Consider removing the initialization loop by handling missing employee names differently in your processing loop. 3. [CHECKLIST ITEM #2] The DateTimeFormatter is created as a local variable inside the method. Move it to a private static final constant at the class level. The line length violations (lines 44-45) and empty lines are style issues that your linter will guide you on fixing. Once these three checklist violations are addressed, your solution should be ready for approval.
✨ 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
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy", Locale.ENGLISH); | ||
| LocalDate localDateFrom = LocalDate.parse(dateFrom, formatter); | ||
| LocalDate localDateTo = LocalDate.parse(dateTo, formatter); | ||
| Map<String, Integer> salaryByName = new HashMap<>(); |
There was a problem hiding this comment.
This violates checklist item #5: 'Don't use two-dimension arrays and HashMap(or any other Map)'. Replace HashMap with a manual approach using parallel arrays or calculated indices.
| public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
|
|
||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy", Locale.ENGLISH); |
There was a problem hiding this comment.
This violates checklist item #2: 'If you create a formatter, make it a constant field'. The DateTimeFormatter should be declared as a static final constant at the class level.
| .append(" - ") | ||
| .append(dateTo); | ||
|
|
||
| for (String name : names) { |
There was a problem hiding this comment.
This violates checklist item #4: 'Don't create redundant loops. Two is enough'. The code uses three loops (initialize map, process data, build result). Consider removing the initialization loop by handling missing names differently.
| } | ||
|
|
||
| if (!localDateTo.isBefore(localDateFrom)) { | ||
| for (String record: data) { |
There was a problem hiding this comment.
This violates checklist item #4. Combined with the initialization loop and result-building loop, this makes three total loops.
| } | ||
| } | ||
|
|
||
| for (String name : names) { |
There was a problem hiding this comment.
This violates checklist item #4. This is the third loop in the method.
| LocalDate localDateFrom = LocalDate.parse(dateFrom, formatter); | ||
| LocalDate localDateTo = LocalDate.parse(dateTo, formatter); | ||
| Map<String, Integer> salaryByName = new HashMap<>(); | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: Remove redundant empty lines within the method.
| .append(dateFrom) | ||
| .append(" - ") | ||
| .append(dateTo); | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: Remove redundant empty lines within the method.
| for (String name : names) { | ||
| salaryByName.put(name, 0); | ||
| } | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: Remove redundant empty lines within the method.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This violates checklist item #1: Remove redundant empty lines within the method.
| .append(name) | ||
| .append(" - ") | ||
| .append(salaryByName.get(name)); | ||
| } |
There was a problem hiding this comment.
This violates checklist item #1: Remove redundant empty lines within the method.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy", Locale.ENGLISH); | ||
| LocalDate localFrom = LocalDate.parse(dateFrom, formatter); | ||
| LocalDate localTo = LocalDate.parse(dateTo, formatter); | ||
| Map<String, Integer> salaryByName = new HashMap<>(); |
There was a problem hiding this comment.
CHECKLIST ITEM #5 violation: HashMap and Map are explicitly prohibited. Replace with parallel arrays approach using calculated indices. For example, use int[] salaries = new int[names.length] and find employee index with a helper method or linear search.
| .append(dateTo); | ||
|
|
||
| for (String name : names) { | ||
| salaryByName.put(name, 0); |
There was a problem hiding this comment.
CHECKLIST ITEM #5 violation: salaryByName.put() uses Map. Replace with array-based approach where you find the employee's index in the names array.
| String recordIncome = recordParts[INCOME_INDEX]; | ||
|
|
||
| if (!recordDate.isBefore(localFrom) && !recordDate.isAfter(localTo)) { | ||
| Integer calculatedIncome = salaryByName.get(recordName) |
There was a problem hiding this comment.
CHECKLIST ITEM #5 violation: salaryByName.get() uses Map. Replace with array-based index lookup.
| Integer calculatedIncome = salaryByName.get(recordName) | ||
| + Integer.parseInt(recordHours) | ||
| * Integer.parseInt(recordIncome); | ||
| salaryByName.put(recordName, calculatedIncome); |
There was a problem hiding this comment.
CHECKLIST ITEM #5 violation: salaryByName.put() uses Map. Replace with array-based index lookup.
| result.append(System.lineSeparator()) | ||
| .append(name) | ||
| .append(" - ") | ||
| .append(salaryByName.get(name)); |
There was a problem hiding this comment.
CHECKLIST ITEM #5 violation: salaryByName.get() uses Map. Replace with array-based index lookup.
|
|
||
| for (String name : names) { | ||
| salaryByName.put(name, 0); | ||
| } | ||
|
|
||
| if (!localTo.isBefore(localFrom)) { | ||
| for (String record: data) { | ||
| if (record != null) { | ||
| String[] recordParts = record.split(" "); | ||
|
|
||
| if (recordParts.length == EXPECTED_PARTS_COUNT) { | ||
| LocalDate recordDate = LocalDate.parse(recordParts[DATE_INDEX], formatter); | ||
| String recordName = recordParts[NAME_INDEX]; | ||
| String recordHours = recordParts[HOURS_INDEX]; | ||
| String recordIncome = recordParts[INCOME_INDEX]; | ||
|
|
||
| if (!recordDate.isBefore(localFrom) && !recordDate.isAfter(localTo)) { | ||
| Integer calculatedIncome = salaryByName.get(recordName) | ||
| + Integer.parseInt(recordHours) | ||
| * Integer.parseInt(recordIncome); | ||
| salaryByName.put(recordName, calculatedIncome); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (String name : names) { | ||
| result.append(System.lineSeparator()) | ||
| .append(name) | ||
| .append(" - ") | ||
| .append(salaryByName.get(name)); |
There was a problem hiding this comment.
CHECKLIST ITEM #4 violation: Code has THREE loops (lines 28-30, 33-51, 54-59). Maximum allowed is TWO. Consider combining the initialization into the processing loop by checking if the employee exists in the names array before calculating.
| public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
|
|
||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy", Locale.ENGLISH); |
There was a problem hiding this comment.
CHECKLIST ITEM #2 violation: DateTimeFormatter is a local variable. Move it to a private static final constant field at class level (after line 13).
| salaryByName.put(name, 0); | ||
| } | ||
|
|
||
| if (!localTo.isBefore(localFrom)) { |
There was a problem hiding this comment.
| if (!localTo.isBefore(localFrom)) { | |
| if (localTo.isAfter(localFrom)) { |
3 participants
No description provided.