Implemented method getSalaryInfo with solution - #1587
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 are a few checklist violations that count as critical requirements for this task. First, your DateTimeFormatter field is final but not static, while the checklist explicitly requires formatters to be true constant fields; please change it to private static final DateTimeFormatter ... and rename it to a Checkstyle-compliant name (the current all-caps FORMATTER triggers AbbreviationAsWordInName and MemberName violations) [CHECKLIST ITEM #2]. Second, you currently append "\n" when building the report; the checklist requires using System.lineSeparator() instead to be cross-platform correct, so replace all hardcoded newline characters with System.lineSeparator() [CHECKLIST ITEM #7]. Third, there is at least one line (around line 14) that exceeds the 100-character limit; break long method calls or expressions onto multiple lines to satisfy the LineLength rule. Functionally, your salary calculation logic, date range handling, and use of LocalDate and StringBuilder look solid, so once these style and checklist-related issues are fixed, your solution should fully meet the requirements. As a minor improvement, consider introducing named constants for the parts[...] indices (date, name, hours, rate) to avoid magic numbers and make the parsing logic more self-documenting [CHECKLIST ITEM #9].
✨ 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
| import java.time.format.DateTimeFormatter; | ||
|
|
||
| public class SalaryInfo { | ||
| private final 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." Here FORMATTER is final but not static; it should be private static final DateTimeFormatter FORMATTER = ... to be a true constant field, consistent with its uppercase name.
| LocalDate from = LocalDate.parse(dateFrom, FORMATTER); | ||
| LocalDate to = LocalDate.parse(dateTo, FORMATTER); | ||
|
|
||
| salaryInfo.append("Report for period ").append(dateFrom).append(" - ").append(dateTo).append("\n"); |
There was a problem hiding this comment.
This violates checklist item #7: "Use System.lineSeparator". Instead of appending "\n", use System.lineSeparator() to ensure correct line separators across OSes.
| for (String name : names) { | ||
| int salary = 0; | ||
| for (String datum : data) { | ||
| String[] parts = datum.split("\\s+"); |
There was a problem hiding this comment.
This likely violates checklist item #9: "Any magic numbers should be constants" because indices parts[0], parts[1], parts[2], parts[3] are hardcoded. Consider introducing named constants for these positions to make the code more self-documenting and avoid magic numbers.
| int salary = 0; | ||
| for (String datum : data) { | ||
| String[] parts = datum.split("\\s+"); | ||
| LocalDate currentDate = LocalDate.parse(parts[0], FORMATTER); |
There was a problem hiding this comment.
Same concern as at line 19 regarding magic numbers: using parts[0] directly without a named constant may violate checklist item #9. Consider defining a constant for the date index.
| for (String datum : data) { | ||
| String[] parts = datum.split("\\s+"); | ||
| LocalDate currentDate = LocalDate.parse(parts[0], FORMATTER); | ||
| if (name.equals(parts[1])) { |
There was a problem hiding this comment.
Using parts[1] directly follows the same magic-number pattern; to comply with checklist item #9, consider using a named constant for the employee name index.
| LocalDate currentDate = LocalDate.parse(parts[0], FORMATTER); | ||
| if (name.equals(parts[1])) { | ||
| if (!currentDate.isBefore(from) && !currentDate.isAfter(to)) { | ||
| salary += Integer.parseInt(parts[2]) * Integer.parseInt(parts[3]); |
There was a problem hiding this comment.
Using parts[2] and parts[3] directly for hours and rate again introduces magic numbers; per checklist item #9, consider extracting these indices as descriptive constants.
| salaryInfo.append(name) | ||
| .append(" - ") | ||
| .append(salary) | ||
| .append("\n"); |
There was a problem hiding this comment.
This also uses a hardcoded "\n", violating checklist item #7 which requires System.lineSeparator() instead of explicit newline characters.
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 fully meets the functional requirements and the specified constraints for the SalaryInfo task. The getSalaryInfo method correctly parses dates with LocalDate and a single private static final DateTimeFormatter, and it handles the date range inclusively when summing salaries. You iterate over names and data with only two nested loops, avoid maps and multidimensional arrays, and compute each employee’s salary as hours * rate exactly as required. The report format matches the expected structure: the header line with the period followed by name - salary lines for each employee, built via StringBuilder and System.lineSeparator(). Magic indices in the parsed parts[] array have been replaced with clear constants, and variable naming and line length comply with common style and Checkstyle rules. Overall, the implementation is clean, correct, and aligned with the checklist, so no further changes are needed on this task.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.