-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
commit salaryInfo #1274
base: master
Are you sure you want to change the base?
commit salaryInfo #1274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have big method, let`s divide it for private
Date entryDate = dateFormat.parse(parts[0]); | ||
if (name.equals(parts[1]) && !entryDate.before(fromDate) | ||
&& !entryDate.after(toDate)) { | ||
int hours = Integer.parseInt(parts[2]); | ||
int rate = Integer.parseInt(parts[3]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's slightly improve your solution ;)
SimpleDateFormat dateFormat = new SimpleDateFormat("dd.MM.yyyy"); | ||
dateFormat.setLenient(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move this local variable to the class fields
List<String> reportLines = new ArrayList<>(); | ||
reportLines.add("Report for period " + dateFrom + " - " + dateTo); | ||
for (String name : names) { | ||
int earned = calculateEarnedSalary(name, data, fromDate, toDate, dateFormat); | ||
reportLines.add(name + " - " + earned); | ||
} | ||
return String.join(System.lineSeparator(), reportLines); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use StringBuilder for these purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's slightly improve your solution ;)
Date fromDate = dateFormat.parse(dateFrom); | ||
Date toDate = dateFormat.parse(dateTo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
dateFormat.setLenient(false); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove try-catch for clarity
if (i < names.length - 1) { | ||
reportLines.append(System.lineSeparator()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of this check, just put lineSeparator at the beginning of each line (and remove it on line 22)
static final int DATE_INDEX = 0; | ||
static final int NAME_INDEX = 1; | ||
static final int HOUR_INDEX = 2; | ||
static final int RATE_INDEX = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget private access modifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more checklist issue
LocalDate fromDate = LocalDate.parse(dateFrom, DateTimeFormatter.ofPattern("dd.MM.yyyy")); | ||
LocalDate toDate = LocalDate.parse(dateTo, DateTimeFormatter.ofPattern("dd.MM.yyyy")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/mate-academy/jv-salary-info/blob/master/checklist.md#if-you-create-a-formatter-make-it-a-constant-field
Sorry I haven't noticed it the first time :)
for (String entry : data) { | ||
String[] parts = entry.split(" "); | ||
LocalDate entryDate = LocalDate.parse(parts[DATE_INDEX], | ||
DateTimeFormatter.ofPattern("dd.MM.yyyy")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice code!
No description provided.