Skip to content

salary - #1589

Open
laubemarcin-sudo wants to merge 7 commits into
mate-academy:masterfrom
laubemarcin-sudo:master
Open

salary#1589
laubemarcin-sudo wants to merge 7 commits into
mate-academy:masterfrom
laubemarcin-sudo:master

Conversation

@laubemarcin-sudo

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 is at least one critical, blocking issue that prevents the code from compiling and fully meeting the task requirements. The most serious problem is the typo name.eguals(fragments[1]) in SalaryInfo.java (around line 19): this must be corrected to name.equals(fragments[1]), otherwise the project will not compile and name matching for salary calculation cannot work. In addition, the DateTimeFormatter is currently created inside the inner loop; per the checklist [CHECKLIST ITEM #2], move it to a private static final DateTimeFormatter field and reuse it. Similarly, parsing dateFrom and dateTo into LocalDate on every inner-loop iteration is redundant work and violates the “avoid redundant operations” idea [CHECKLIST ITEM #4]; instead, parse them once before the loops into fromDate and toDate and reuse those variables. There are also magic indices like fragments[2] and fragments[3] which would be clearer as named private static final constants (e.g., HOURS_INDEX, RATE_INDEX), and renaming fragments to something like recordParts would make the code more self-explanatory [CHECKLIST ITEM #8, #9]. Once you fix the compilation error and move the formatter and date parsing out of the inner loop, your solution should both compile and better align with the checklist; the overall structure and logic are already on the right track.


✨ 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


for (String record : data) {
String[] fragments = record.split(" ");
if (name.eguals(fragments[1])) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

name.eguals(fragments[1]) contains a typo (eguals instead of equals), which will not compile and prevents matching names; fix this to use name.equals(...) to satisfy the core salary-calculation requirement.

for (String record : data) {
String[] fragments = record.split(" ");
if (name.eguals(fragments[1])) {
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.

Creating DateTimeFormatter inside the inner loop violates checklist item #2: "If you create a formatter, make it a constant field." Move this formatter to a private static final DateTimeFormatter field and reuse it.

if (name.eguals(fragments[1])) {
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy");
LocalDate recordDate = LocalDate.parse(fragments[0], formatter);
LocalDate fromDate = LocalDate.parse(dateFrom, formatter);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Parsing dateFrom into fromDate on every iteration of the inner loop is redundant work; per checklist item #4 about avoiding redundant loops/operations, compute fromDate once before the loops and reuse it.

DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy");
LocalDate recordDate = LocalDate.parse(fragments[0], formatter);
LocalDate fromDate = LocalDate.parse(dateFrom, formatter);
LocalDate toDate = LocalDate.parse(dateTo, formatter);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similarly, parsing dateTo into toDate on every iteration is redundant; move this parsing outside the inner loop and reuse the same LocalDate instance to avoid unnecessary repeated work.

LocalDate toDate = LocalDate.parse(dateTo, formatter);

if (!recordDate.isBefore(fromDate) && !recordDate.isAfter(toDate)) {
int hours = Integer.parseInt(fragments[2]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The indices 2 and 3 used to access fragments are magic numbers; this violates checklist item #9: "Any magic numbers should be constants". Consider extracting them into named private static final constants to make the code clearer.


if (!recordDate.isBefore(fromDate) && !recordDate.isAfter(toDate)) {
int hours = Integer.parseInt(fragments[2]);
int rate = Integer.parseInt(fragments[3]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same as above, using index 3 directly is a magic number; define a named constant for the rate index to comply with checklist item #9.

int salary = 0;

for (String record : data) {
String[] fragments = record.split(" ");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable name fragments is somewhat abstract and not very informative; this touches checklist item #8 about informative names. Consider a clearer name like recordParts or dataParts to express what the array contains.

@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 it fully meets the functional and technical requirements of the task. The getSalaryInfo method correctly uses LocalDate and an inclusive date range to calculate salaries, and it parses dateFrom and dateTo only once before the loops, which is both efficient and clean. Your implementation iterates over the names array as the outer loop, processes the data records appropriately, and computes hours * rate per employee without using forbidden structures like Map or 2D arrays. The output format matches the required specification: the header line with the period and each subsequent line in the Name - salary form, using StringBuilder and System.lineSeparator(). Magic indices are replaced with well-named constants, and the previous typo in the equality check has been fixed, so there are no remaining critical or blocking issues. Nicely done keeping the solution simple, readable, and aligned with all checklist constraints.


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

Footnotes

  1. Rate AI review example

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