Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implemented method getSalaryInfo #1347

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

OlehRidchenko
Copy link

No description provided.

Copy link

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

Choose a reason for hiding this comment

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

Great job on the Pull Request! Just a couple of minor suggestions: consider optimizing the way you handle line breaks with StringBuilder and use System.lineSeparator() for better cross-platform support. These changes aren't critical, but they'll make your code more robust and maintainable. Keep up the good work! 😊👍

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


message.append("Report for period ")
.append(dateFrom).append(" - ")
.append(dateTo).append("\r\n");

Choose a reason for hiding this comment

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

Instead of using '\r\n' for line breaks, use 'System.lineSeparator()' for better compatibility across different operating systems.

totalSalary += hoursWorked * salaryPerHour;
}
}
message.append(name).append(" - ").append(totalSalary).append("\r\n");

Choose a reason for hiding this comment

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

Use 'System.lineSeparator()' instead of '\r\n' when appending the salary information to ensure the line separator is handled correctly across various operating systems.

Comment on lines 40 to 41
message.deleteCharAt(message.length() - 1);
message.deleteCharAt(message.length() - 1);

Choose a reason for hiding this comment

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

Instead of manually deleting the last characters to remove the extra line break, consider using 'StringBuilder' methods more effectively. For example, you could avoid appending the extra line separator after the last record, or use 'StringBuilder.setLength' to trim the 'StringBuilder' to the correct length.

Copy link

@ahoienko ahoienko left a comment

Choose a reason for hiding this comment

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

Please make CI successful first

Copy link

@liliia-ponomarenko liliia-ponomarenko left a comment

Choose a reason for hiding this comment

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

Good job! A few recommendations ;)

StringBuilder message = new StringBuilder();

message.append("Report for period ")
.append(dateFrom).append(" - ")

Choose a reason for hiding this comment

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

hyphen also should make constant

Comment on lines +32 to +34
if (employeeName.equals(name)
&& (workDate.isEqual(startDate) || workDate.isEqual(endDate)
|| (workDate.isAfter(startDate) && workDate.isBefore(endDate)))) {

Choose a reason for hiding this comment

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

you can move this conditional to private method and make your method more readable;)

}
message.append(name).append(" - ").append(totalSalary).append(System.lineSeparator());
}
message.setLength(message.length() - System.lineSeparator().length());

Choose a reason for hiding this comment

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

why do you need it?

Copy link
Author

Choose a reason for hiding this comment

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

to remove line separator after outputing last element

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.

4 participants