Implemented method getSalaryInfo(String[] names, String[] data, Strin… - #1594
Implemented method getSalaryInfo(String[] names, String[] data, Strin…#1594LizaBrnk wants to merge 4 commits into
Conversation
…g dateFrom, String dateTo)
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is functionally correct and meets the core requirements for salary calculation, date filtering, and output formatting. However, there are two explicit checklist violations that need to be addressed:
-
DateTimeFormatter as local variable [CHECKLIST ITEM #2]: The
DateTimeFormatteris declared inside the method. Move it to the top of the class as aprivate static finalfield instead. -
Magic number "dd.MM.yyyy" [CHECKLIST ITEM #9]: The date format pattern is hardcoded inline. Declare it as a constant with an informative name like
DATE_FORMAT_PATTERNat the class level.
These are straightforward fixes—just two additional static final constants at the top of your class. Once you extract these values into proper constants, your code will fully comply with the requirements.
✨ 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; | ||
| StringBuilder result = new StringBuilder(); | ||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
According to checklist item #2, if you create a DateTimeFormatter, it should be a constant field, not a local variable. Move it to the top of the class as a static final field.
| public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
| StringBuilder result = new StringBuilder(); | ||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
The date format pattern "dd.MM.yyyy" is a magic number. According to checklist item #9, it should be declared as a constant with an informative name (e.g., DATE_FORMAT_PATTERN).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
All core requirements are met, including proper use of LocalDate, StringBuilder, System.lineSeparator(), correct salary calculation logic, and inclusive date filtering. Both DateTimeFormatter and the date format pattern "dd.MM.yyyy" are correctly declared as private static final constants. There is one minor style issue: line 8 in SalaryInfo.java exceeds the maximum line length of 100 characters (104 characters found). This is a checkstyle violation that will need to be addressed before the CI pipeline passes, but it doesn't affect functionality. Break the line appropriately - for example, if it's a long class declaration, split the extends or implements clauses across multiple lines with proper indentation.
✨ 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 correctly fulfills the core functional requirements using the Java 8 time API and proper date filtering logic. However, there are two checklist items to address: Line 8 exceeds 100 characters (104 characters) — consider splitting the DateTimeFormatter declaration across multiple lines with proper indentation, similar to your method parameters formatting on lines 11-14. Additionally, the space character " " used for splitting data strings is a magic number that should be extracted as a private static final String DELIMITER = " "; constant, as required by checklist item #8. This is an educational override to help you progress while learning — fixing these formatting and style conventions will strengthen your code quality skills for future projects!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
| private static final String DATE_FORMAT_PATTERN = "dd.MM.yyyy"; | ||
| private static final DateTimeFormatter formatter |
There was a problem hiding this comment.
Line 8 exceeds 100 characters (104 characters). Break the formatter assignment across multiple lines with proper indentation, similar to how method parameters are formatted on lines 11-14.
| int salary = 0; | ||
|
|
||
| for (String line : data) { | ||
| String[] info = line.split(" "); |
There was a problem hiding this comment.
The magic number " " (space character used for splitting data strings) should be extracted as a private static final constant with an informative name, as required by checklist item #8.
2 participants
…g dateFrom, String dateTo)