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

Implement method to calculate employee salaries #1282

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

Conversation

smagles
Copy link

@smagles smagles commented May 22, 2024

No description provided.

Copy link

@MateuszRostek MateuszRostek left a comment

Choose a reason for hiding this comment

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

Nice try! Before creating a PR please go through all the common mistakes provided at the end of the task description, I left some comments, please implement them and then before re-requesting review make sure you don't make these most common mistakes.

https://github.com/mate-academy/jv-salary-info/blob/master/checklist.md

return salaryMap;
}

private String print(Map<String, Integer> salaryMap,

Choose a reason for hiding this comment

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

This name is a little misleading, print usually means to print something on a console for example and don't return any value. Please change this name to something like createReport.

public class SalaryInfo {
private final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy");

Choose a reason for hiding this comment

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

make it a constant (private static final)

LocalDate startDate = parseDate(dateFrom);
LocalDate endDate = parseDate(dateTo);

Map<String, Integer> salaryMap = initializeSalaryMap(names);

Choose a reason for hiding this comment

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

image

StringBuilder result = new StringBuilder();
result.append("Report for period ")
.append(dateFrom).append(" - ")
.append(dateTo).append("\n");

Choose a reason for hiding this comment

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

image

Comment on lines 21 to 24
LocalDate date = LocalDate.parse(parts[0], formatter);
String name = parts[1];
int workHours = Integer.parseInt(parts[2]);
int perHourRate = Integer.parseInt(parts[3]);

Choose a reason for hiding this comment

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

image

Choose a reason for hiding this comment

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

Please try to do it with 2 loops, right now you use 2 normal for loops and one forEach on your map.

image

Copy link

@MateuszRostek MateuszRostek 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, approved! See minor advice.

}

} catch (Exception e) {
System.err.println("Invalid data entry: " + entry);

Choose a reason for hiding this comment

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

If you want to catch an exception here, you should re-throw some custom exception/use RuntimeException.

Suggested change
System.err.println("Invalid data entry: " + entry);
throw new RuntimeException("Invalid data entry: " + entry, e);

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