Release getSalaryInfo - #1596
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The code has critical compilation errors that must be fixed before approval. The main issue is that private static final constants (REGEX, INDEX_DATE, INDEX_NAME, INDEX_WORKING_HOUR, INDEX_SALARY, DATE_FORMAT) are declared inside the getSalaryInfo() method body (lines 8-13) — this is invalid Java syntax, as static fields must be declared at the class level. The build is failing due to these syntax errors. Additionally, the DateTimeFormatter needs to be moved to class level as a private static final constant field per checklist item #2. Required fixes: 1. Move all constants (REGEX, INDEX_DATE, INDEX_NAME, INDEX_WORKING_HOUR, INDEX_SALARY, DATE_FORMAT) to class-level (after line 6); 2. Move DateTimeFormatter to class-level as private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern(DATE_FORMAT);; 3. Remove the redundant if (sumSalary >= 0) check (it's always true since sumSalary starts at 0 and only increases). Minor improvements: Consider renaming datum and parsDatum to more descriptive names like dataEntry or record per checklist item #8. The salary calculation logic and date range filtering appear correct once the syntax issues are resolved.
✨ 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
| private static final String REGEX = "\\s+"; | ||
| private static final int INDEX_DATE = 0; | ||
| private static final int INDEX_NAME = 1; | ||
| private static final int INDEX_WORKING_HOUR = 2; | ||
| private static final int INDEX_SALARY = 3; | ||
| private static final String DATE_FORMAT = "dd.MM.yyyy"; |
There was a problem hiding this comment.
Static fields cannot be declared inside a method. Lines 8-13 declare private static final constants inside getSalaryInfo(). Move these to class-level (after line 6) to fix the syntax error.
| private static final int INDEX_WORKING_HOUR = 2; | ||
| private static final int INDEX_SALARY = 3; | ||
| private static final String DATE_FORMAT = "dd.MM.yyyy"; | ||
| DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(DATE_FORMAT); |
There was a problem hiding this comment.
According to checklist item #2, if a DateTimeFormatter is created, it must be a constant field. Move dateTimeFormatter to class-level as private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern(DATE_FORMAT);
| stringBuilder.append("Report for period ").append(dateFrom).append(" - ").append(dateTo); | ||
| for (String name : names) { | ||
| int sumSalary = 0; | ||
| for (String datum : data) { |
There was a problem hiding this comment.
According to checklist item #8, do not use abstract words like 'datum' as variable names. Use an informative name like 'dataEntry' or 'record' instead.
| * Integer.parseInt(parsDatum[INDEX_SALARY]); | ||
| } | ||
| } | ||
| if (sumSalary >= 0) { |
There was a problem hiding this comment.
The condition if (sumSalary >= 0) is redundant since sumSalary is initialized to 0 and only incremented with positive values. Remove this check.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation's core logic is correct and functional, but two explicit checklist violations remain. First, the DateTimeFormatter at class level is missing the final modifier [CHECKLIST #2] - it must be private static final DateTimeFormatter. Second, the regex pattern and magic number index values (indexDate, indexName, indexWorkingHour, indexSalary) are declared inside the method body instead of as private static final class-level constants [CHECKLIST #9]. These are straightforward constant declarations that follow standard Java conventions for values used throughout the class.
✨ 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 static DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
The DateTimeFormatter should be declared as private static final per checklist item #2. Add the final keyword to make it a constant field.
|
|
||
| public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
| return null; | ||
| String regex = "\\s+"; |
There was a problem hiding this comment.
The regex pattern is a constant used throughout the method. Per checklist item #9, extract magic numbers and hardcoded values to constant fields at class level.
| int indexDate = 0; | ||
| int indexName = 1; | ||
| int indexWorkingHour = 2; | ||
| int indexSalary = 3; |
There was a problem hiding this comment.
Index values 0, 1, 2, 3 are magic numbers. Per checklist item #9, extract these to named constant fields at class level with informative names like INDEX_DATE, INDEX_NAME, etc.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is excellent and meets all requirements. You correctly added the final modifier to DateTimeFormatter, moved all magic numbers to private static final constants, and used proper date handling with LocalDate and DateTimeFormatter.ofPattern("dd.MM.yyyy"). The nested loop structure, StringBuilder for string concatenation, and System.lineSeparator() usage are all spot-on. Great job on the informative variable naming and following the required output format!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.