Skip to content

sallary info solution - #1582

Open
rubashnyi wants to merge 2 commits into
mate-academy:masterfrom
rubashnyi:master
Open

sallary info solution#1582
rubashnyi wants to merge 2 commits into
mate-academy:masterfrom
rubashnyi:master

Conversation

@rubashnyi

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are a few critical checklist violations that need to be fixed before this can be considered complete, even though the core salary calculation logic is correct. First, in src/main/java/core/basesyntax/SalaryInfo.java you are using a HashMap/Map to store salaries, which is explicitly forbidden by the task requirements; instead, you should accumulate salaries using arrays or variables indexed by the names array so that no Map implementations are used. Second, the date formatter is an instance (non-constant) field; per the checklist, it must be declared as a constant, e.g. private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("dd.MM.yyyy");, and then used throughout the class. Third, there is a redundant empty line after the package (and/or imports) before the class/first field declaration, which violates the “don’t begin class or method implementation with an empty line” rule and needs to be removed. As a minor improvement, you could also replace the magic indices like parts[0], parts[1], etc. with named constants (e.g. DATE_INDEX, NAME_INDEX) to make the code more readable. Your overall logic, use of LocalDate, inclusive date filtering, and StringBuilder formatting look good, so once you adjust the data structure, make the formatter a static final constant, and clean up the extra blank line, your solution should be in good shape.


✨ 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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +5 to +6
import java.util.HashMap;
import java.util.Map;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item: Don’t use two-dimension arrays and HashMap(or any other Map). You are importing and using HashMap/Map to track salaries, but the task requires solving it without any Map implementations.


public class SalaryInfo {

private final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item: If you create a formatter, make it a constant field. The formatter here is an instance field and not a constant (should be private static final DateTimeFormatter FORMATTER = ...; with constant-style naming).

import java.util.HashMap;
import java.util.Map;

public class SalaryInfo {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line shows the class starting after an empty line following the package declaration, which conflicts with the checklist requirement: Don't begin class or method implementation with an empty line. Remove all redundant empty lines.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved because all functional and task requirements are now correctly implemented and there are no critical blockers. The getSalaryInfo method correctly uses LocalDate with a constant DateTimeFormatter, applies the date range inclusively, and computes salaries per employee from the given names and data arrays without using forbidden structures like Map or two-dimensional arrays. The output format matches the required report header and per-employee lines, and the sample values (e.g., 900/1050/1800) will be produced as expected. Variable naming, use of StringBuilder, and extraction of magic indices into constants all show good adherence to clean code practices.

The only remaining issue is a minor style detail: there is still an extra blank line after the package declaration in SalaryInfo.java, before the import statements. This violates the style checklist item about not beginning the class implementation with an empty line, but it is not critical for functionality and can be easily fixed in a follow-up formatting pass or by your linter. Overall, you handled the earlier review comments well and the core logic and structure of your solution are solid.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

@@ -1,7 +1,62 @@
package core.basesyntax;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines". There is an empty line between the package declaration and the import statements; remove that redundant blank line so the class doesn’t start with an empty line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants