complete task - #1590
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is at least one critical, functionality-breaking issue and a couple of explicit checklist violations that must be fixed before this solution is safe to use. First, in SalaryInfo.java, the expression Arrays.asList(names).indexOf(name) can return -1 when a record in data contains a name that is not present in the names array; using that value to index into salaries[index] will cause an ArrayIndexOutOfBoundsException, so you need to guard against index == -1 before updating the salary (e.g., skip such records) to avoid a runtime error. Second, your DateTimeFormatter is declared as a local variable inside the method, which violates the requirement that formatters be constant fields [CHECKLIST ITEM #2]; please move it to a private static final DateTimeFormatter at the class level and reuse it. Third, you directly use numeric indices such as parts[2] and parts[3] (hours and rate) and rely on the implicit record length 4, which are considered magic numbers [CHECKLIST ITEM #9]; extract these into well-named private static final constants like DATE_INDEX, NAME_INDEX, HOURS_INDEX, and RATE_INDEX to make the parsing clearer and compliant.
Functionally, your overall logic, use of LocalDate, StringBuilder, and System.lineSeparator(), and adherence to using only two loops are all good and match the task requirements, so you are close to a solid solution. Once you fix the unsafe name lookup, move the formatter to a constant, and replace the magic indices with constants, your code should be both robust and fully aligned with the checklist. Keep up the good work—these adjustments are mostly about making your implementation safer and more professional.
✨ 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 class SalaryInfo { | ||
| public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
This violates checklist item #2: If you create a formatter, make it a constant field. You currently declare DateTimeFormatter formatter inside the method. Consider moving it to a private static final field of the class.
| int rate = Integer.parseInt(parts[3]); | ||
| LocalDate recordDate = LocalDate.parse(date, formatter); | ||
| if (!recordDate.isBefore(from) && !recordDate.isAfter(to)) { | ||
| int index = Arrays.asList(names).indexOf(name); |
There was a problem hiding this comment.
There is a potential bug here: if name from data is not present in the names array, Arrays.asList(names).indexOf(name) returns -1, causing salaries[-1] access and an ArrayIndexOutOfBoundsException. The task description doesn’t guarantee that all names in data are in names, so you should guard against index == -1 before using it.
| String[] parts = record.split(" "); | ||
| String date = parts[0]; | ||
| String name = parts[1]; | ||
| int hours = Integer.parseInt(parts[2]); |
There was a problem hiding this comment.
This uses a magic number (2 index for hours) directly. Checklist item #9 requires: Any magic numbers should be constants. Consider extracting indices like 0, 1, 2, 3 into well-named constant fields (e.g., DATE_INDEX, NAME_INDEX, HOURS_INDEX, RATE_INDEX).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a direct violation of the explicit formatter checklist requirement, which is considered a critical blocker for this task. In src/main/java/core/basesyntax/SalaryInfo.java, your DateTimeFormatter is currently declared as a local variable inside getSalaryInfo, which conflicts with [CHECKLIST ITEM #2] that requires any formatter you create to be a constant field. To fix this, move the formatter declaration to the class level as something like private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("dd.MM.yyyy"); and then use that constant inside getSalaryInfo instead of creating a new local formatter. Functionally, your salary calculations, date range handling, and use of LocalDate, StringBuilder, constants for indices, and guarding index >= 0 all look solid and meet the task requirements. Once you extract the formatter to a constant field with proper naming, your solution should fully comply with both the functional and checklist requirements, so this is the only required change before it can be accepted.
✨ 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 String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
This violates checklist item #2: If you create a formatter, make it a constant field. If you create a formatter, make it a constant. – currently formatter is a local variable inside the method; consider moving it to a private static final DateTimeFormatter at the class level and using that here.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it correctly computes each employee’s salary within the given inclusive date range and formats the result exactly as required. You’ve done a great job using LocalDate with a class-level DateTimeFormatter constant, avoiding disallowed structures like maps or 2D arrays, and keeping the logic to just two clear loops. The use of StringBuilder, named index constants for parsing, and System.lineSeparator() shows careful attention to both performance and style requirements. Variable and method names are descriptive, which makes the code easy to read and maintain. Overall, your implementation meets all functional and checklist requirements, and you can confidently move forward with this solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
4 participants
No description provided.