new check - #1602
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has three critical checklist violations that must be fixed before approval. First, you're using LinkedHashMap which violates checklist item #5 — the task explicitly prohibits using any Map implementation. Second, you use '\n' for line breaks instead of System.lineSeparator() (checklist #7). Third, and most importantly, your names parameter is completely ignored — the method should only calculate and return salary information for employees listed in the names array, but currently all employees from data appear in output regardless. The checkstyle issues (star imports, whitespace, line length) are style violations that you'll need to fix separately, but they're not blocking approval. Focus on removing the Map entirely and implementing proper filtering by the names array.
✨ 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
Your implementation has two critical issues that need fixing. First, you have three loops (lines 19, 23, and 32) but checklist item #4 explicitly requires a maximum of two loops. The output generation loop at line 32 can be merged with the salary accumulation loop at line 19 - since you already iterate through the employee names inside that loop, you can build the report string there as well instead of having a separate loop. Second, there are empty lines at the beginning of the class (lines 2, 5, 9) which violate checklist item #1 - remove these blank lines at the start of class and method implementations.
✨ 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
| @@ -1,7 +1,40 @@ | |||
| package core.basesyntax; | |||
|
|
|||
There was a problem hiding this comment.
Remove this empty line - checklist item #1 prohibits empty lines at the beginning of class implementation.
|
|
||
| import java.time.LocalDate; | ||
| import java.time.format.DateTimeFormatter; | ||
|
|
There was a problem hiding this comment.
Remove this empty line - checklist item #1 prohibits empty lines at the beginning of class implementation.
| public class SalaryInfo { | ||
| private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); | ||
| private static final String REPORT = "Report for period "; | ||
|
|
There was a problem hiding this comment.
Remove this empty line - checklist item #1 prohibits empty lines at the beginning of method implementation.
| } | ||
| } | ||
| } | ||
| for (int k = 0; k < salary.length; k++) { |
There was a problem hiding this comment.
This is the third loop - checklist item #4 requires maximum two loops. The output generation can be merged with the salary calculation loop. Since you already iterate through names inside the data loop, you can build the output string there instead of having a separate loop.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation correctly fulfills all task requirements with proper date handling, two-loop structure, and StringBuilder usage. However, there are two checkstyle violations that need fixing before final approval: Line 1 needs a trailing newline at the end of the file (ensure the last line is followed by an empty line), and Line 15 exceeds 100 characters (107 found) - split this line across multiple lines to improve readability. These are straightforward formatting fixes that won't affect your logic. Great work on meeting all the core requirements - keep practicing these formatting conventions!
✨ 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
slade13
left a comment
slade13
left a comment
There was a problem hiding this comment.
Good job, please review my comments. Overall, the solution is good and correctly implements the required salary calculation. The remaining issues are mainly code-style and maintainability improvements rather than problems with the algorithm.
| import java.time.format.DateTimeFormatter; | ||
|
|
||
| public class SalaryInfo { | ||
| private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
formatter should be renamed to follow constant naming conventions, for example DATE_FORMATTER. It is declared as static final, so according to the checklist its name should be uppercase.
| private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); | |
| private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
| } | ||
| } | ||
| report.append(name).append(" - ").append(salary); | ||
| if (!name.equals(names[names.length - 1])) { |
There was a problem hiding this comment.
Avoid checking whether the current employee is the last one using: name.equals(names[names.length - 1]).
This can give incorrect formatting if the names array contains duplicate names. A safer approach is to build each employee line consistently and remove the final line separator at the end, or iterate using an index.
| import java.time.LocalDate; | ||
| import java.time.format.DateTimeFormatter; | ||
|
|
||
| public class SalaryInfo { |
There was a problem hiding this comment.
Variable names such as from, to, datum, dataArray, and localDate could be more descriptive. For example, dateFrom, dateTo, dataRecord, recordParts, and workDate would make the logic easier to understand.
| String[] dataArray = datum.split(" "); | ||
| LocalDate localDate = LocalDate.parse(dataArray[0], formatter); | ||
| if (!localDate.isBefore(from) | ||
| && !localDate.isAfter(to) | ||
| && name.equals(dataArray[1])) { | ||
| salary += Integer.parseInt(dataArray[2]) | ||
| * Integer.parseInt(dataArray[3]); | ||
| } |
There was a problem hiding this comment.
Consider extracting the indexes from dataArray into constants instead of using magic numbers 0, 1, 2, and 3. These positions represent date, employee name, worked hours, and hourly rate, so named constants would improve readability.
3 participants
No description provided.