Implement SalaryInfo solution - #1585
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution correctly implements all core functional requirements: it computes salaries within the given date range, uses LocalDate, StringBuilder, and System.lineSeparator, and follows the required loop structure without using Maps or 2D arrays. The output format for the report header and the per-employee lines matches the specification, so the functionality itself is solid and meets the task goals. The remaining issues are style/checklist concerns: the DateTimeFormatter should be moved to a private static final constant, the header string fragments (like "Report for period " and " - ") should be extracted into named constants, and the extra blank line after the package declaration should be removed. These are important for clean, maintainable code, but they don't break functionality and can be fixed easily in a follow-up pass or by addressing linter warnings. Overall, your logic and structure are good; just remember to factor out magic strings and formatters into constants and keep an eye on small style details as you refine your code.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
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: If you create a formatter, make it a constant field. You should declare the DateTimeFormatter as a private static final constant in the class instead of creating it inside the method.
| LocalDate fromDate = LocalDate.parse(dateFrom, formatter); | ||
| LocalDate toDate = LocalDate.parse(dateTo, formatter); | ||
|
|
||
| StringBuilder salaryInfo = new StringBuilder("Report for period " |
There was a problem hiding this comment.
This line participates in constructing the header using hardcoded string literals. This violates checklist item: Any magic numbers should be constants (applied also to hardcoded strings). Consider extracting the header prefix (e.g. "Report for period " and the separator " - ") into private static final string constants.
| LocalDate toDate = LocalDate.parse(dateTo, formatter); | ||
|
|
||
| StringBuilder salaryInfo = new StringBuilder("Report for period " | ||
| + dateFrom + " - " + dateTo); |
There was a problem hiding this comment.
Similar to line 12, this continues building the header with inline string values. Per checklist item Any magic numbers should be constants, move these hardcoded strings into well-named constant fields.
| @@ -1,7 +1,38 @@ | |||
| package core.basesyntax; | |||
|
|
|||
There was a problem hiding this comment.
There is an empty line between the package statement and the import, meaning the class effectively begins with an empty line after the package. This goes against checklist item: Don't begin class or method implementation with an empty line. Remove all redundant empty lines. Remove the redundant blank line after the package declaration.
2 participants
No description provided.