-
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
create an implementation for the method "getSalaryInfo" #1290
base: master
Are you sure you want to change the base?
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.
Good job overall!
String recordDate = record[0]; | ||
String recordName = record[1]; | ||
int totalHours = Integer.parseInt(record[2]); | ||
int salaryPerHour = Integer.parseInt(record[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.
indexes to constants
import java.time.format.DateTimeParseException; | ||
|
||
public class SalaryInfo extends DataComparison { | ||
|
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 leave an empty line after method/class definition
return null; | ||
import java.time.format.DateTimeParseException; | ||
|
||
public class SalaryInfo extends DataComparison { |
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.
why do you extend datacomparison? it looks like just a utility class, make its method static and use directly
return ((dateToCheck.isEqual(startDate) || dateToCheck.isAfter(startDate)) | ||
&& (dateToCheck.isEqual(endDate) || dateToCheck.isBefore(endDate))); |
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 can shorten it to just two parts instead of 4 (using !)
return 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.
|
||
try { | ||
for (String date : dates) { | ||
String[] record = date.split("\\s+"); |
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 create REGEX constant
for (int i = 0; i < names.length; i++) { | ||
builder.append(names[i]).append(" - ").append(employeeSalary[i]) | ||
.append(System.lineSeparator()); | ||
} | ||
return builder.toString().trim(); |
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.
that's a third loop - two are enough (common mistakes checklist)
… it`s method to the SalaryInfo class and made it static, made 2 loops instead of 3 ones, and made date checking shorter
private static final int HOURS_RATE_FROM_LIST = 3; | ||
private static final String LINE_SEPARATOR = "\\s+"; | ||
|
||
public static boolean isDateInRange(String fromDate, String toDate, String dateFromAList) { |
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.
Looks like this method should be private and placed after public methods
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 fix comment
No description provided.