-
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
01-jv-salary-info #1330
base: master
Are you sure you want to change the base?
01-jv-salary-info #1330
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.
Great job on your latest code submission! 👍 It looks like you've addressed the main functional requirements and there's nothing that critically impacts the functionality or security. Just a few pointers for next time: consider using constants for magic numbers and string literals to make your code clearer, and keep using StringBuilder
for string concatenation in loops for better performance. Also, try to use more descriptive variable names and refactor your code to avoid mimicking the functionality of maps with nested loops. Keep up the good work, and remember, small improvements can make a big difference in the long run! 😊👨💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
return null; | ||
public static String getSalaryInfo(String[] names, String[] data, | ||
String dateFrom, String dateTo) { | ||
String dateFromComparable = convertDateToComparable(dateFrom); |
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.
If you create a formatter, make it a constant field. This will help with maintainability and performance if the formatter is used multiple times.
String dateFrom, String dateTo) { | ||
String dateFromComparable = convertDateToComparable(dateFrom); | ||
String dateToComparable = convertDateToComparable(dateTo); | ||
int[] totalEarnings = new int[names.length]; |
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 begin class or method implementation with an empty line.
return report.toString(); | ||
} | ||
|
||
private static String convertDateToComparable(String date) { |
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.
This method name 'convertDateToComparable' is not very informative. The name should reflect that it's converting a date to a string format that can be compared, something like 'formatDateForComparison' might be clearer.
for (String entry : data) { | ||
String[] parts = entry.split(" "); | ||
String dateStr = parts[0]; | ||
String employeeName = parts[1]; | ||
int hoursWorked = Integer.parseInt(parts[2]); | ||
int incomePerHour = Integer.parseInt(parts[3]); | ||
|
||
String entryDateComparable = convertDateToComparable(dateStr); | ||
|
||
if (entryDateComparable.compareTo(dateFromComparable) | ||
>= 0 && entryDateComparable.compareTo(dateToComparable) <= 0) { | ||
for (int i = 0; i < names.length; i++) { | ||
if (names[i].equals(employeeName)) { | ||
int earnings = hoursWorked * incomePerHour; | ||
totalEarnings[i] += earnings; | ||
break; | ||
} | ||
} | ||
} |
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 use two-dimension arrays and HashMap (or any other Map). Try to solve the task without using these data structures. Although you aren't using them here, the nested loop on lines 21-27 is essentially mimicking a map's functionality. Consider refactoring to avoid the inner loop.
String dateToComparable = convertDateToComparable(dateTo); | ||
int[] totalEarnings = new int[names.length]; | ||
|
||
for (String entry : data) { |
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.
Remember about informative names of variables and methods. The variable 'data' is too generic. A name like 'records' or 'workLogs' would be more descriptive.
return report.toString(); | ||
} | ||
|
||
private static String convertDateToComparable(String date) { |
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.
Use LocalDate instead of Date. This method is currently relying on string manipulation for date comparison, which is less reliable and less readable than using LocalDate.
StringBuilder report = new StringBuilder(); | ||
report.append("Report for period ").append(dateFrom) | ||
.append(" - ").append(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.
Use StringBuilder for appending strings in loops. You're already using StringBuilder, which is good, but make sure to use it consistently for all string concatenations within loops.
} | ||
} | ||
|
||
StringBuilder report = new StringBuilder(); |
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.
Any magic numbers should be constants. The string literals in the report generation, such as 'Report for period ', should be extracted to constant fields with informative names.
String dateStr = parts[0]; | ||
String employeeName = parts[1]; | ||
int hoursWorked = Integer.parseInt(parts[2]); | ||
int incomePerHour = 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.
all magic numbers should be constants
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, but it is possible to improve your solution)
static final int iDateStr = 0; | ||
static final int iEmployeeName = 1; | ||
static final int iHoursWorked = 2; | ||
static final int intIncomePerHour = 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.
Constants should have special naming convetion, and please don`t forger about access modifier
|
||
private static String convertDateToComparable(String date) { | ||
String[] parts = date.split("\\."); | ||
return parts[2] + parts[1] + parts[0]; |
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.
not fixed
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
static final int iDateStr = 0; | ||
static final int iEmployeeName = 1; |
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.
What 'i' means in name of variable? Better to avoid abbreviation if it is not common
StringBuilder report = new StringBuilder(); | ||
report.append("Report for period ").append(dateFrom) | ||
.append(" - ").append(dateTo); | ||
|
||
for (int i = 0; i < names.length; i++) { | ||
report.append(System.lineSeparator()).append(names[i]).append(" - ") | ||
.append(totalEarnings[i]); | ||
} | ||
|
||
return report.toString(); |
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.
This method (getSalaryInfo) is pretty long and makes a lot of things, so I suggest you to break him down to smaller methods. For example logic of report creating could be moved to separate method
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.
Two loops are enough
make sure not to have any of these
https://github.com/mate-academy/jv-salary-info/blob/master/checklist.md
static final int intDateStr = 0; | ||
static final int intEmployeeName = 1; | ||
static final int intHoursWorked = 2; | ||
static final int intIncomePerHour = 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.
add private
|
||
private static String convertDateToComparable(String date) { | ||
String[] parts = date.split("\\."); | ||
return parts[2] + parts[1] + parts[0]; |
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.
not fixed
if (entryDateComparable.compareTo(dateFromComparable) >= 0 | ||
&& entryDateComparable.compareTo(dateToComparable) <= 0) { |
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 use compareTO, use isBefore, isAfter
private static final DateTimeFormatter date_formatter = | ||
DateTimeFormatter.ofPattern("dd.MM.yyyy"); | ||
|
||
public static String getSalaryInfo(String[] names, String[] data, |
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.
this method and the next ones shouldn't be static
private static final int intDateStr = 0; | ||
private static final int intEmployeeName = 1; | ||
private static final int intHoursWorked = 2; | ||
private static final int intIncomePerHour = 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.
fix names to INCOME_PER_HOUR_INDEX
for (int i = 0; i < names.length; i++) { | ||
report.append(System.lineSeparator()).append(names[i]).append(" - ") | ||
.append(totalEarnings[i]); | ||
} | ||
return report.toString(); |
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.
integrate this into main public method, you still have 3 for loops
report.append("Report for period ").append(dateFrom) | ||
.append(" - ").append(dateTo); | ||
|
||
for (int i = 0; i < names.length; i++) { |
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.
third loop, try to do it in two loops
private static final int EMPLOYEE_NAME_INDEX = 1; | ||
private static final int HOURS_WORKED_INDEX = 2; | ||
private static final int INCOME_PER_HOUR_INDEX = 3; | ||
private static final DateTimeFormatter date_formatter = |
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.
bad naming for a constant
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.
date_formatter =bad naming for a constant
github preview for a comment can be confusing, always look at the latest line of preview code block,I was commenting line 11
private static final int EMPLOYEE_NAME_INDEX = 1; | ||
private static final int HOURS_WORKED_INDEX = 2; | ||
private static final int INCOME_PER_HOUR_INDEX = 3; | ||
private static final DateTimeFormatter date_formatter = |
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.
date_formatter =bad naming for a constant
github preview for a comment can be confusing, always look at the latest line of preview code block,I was commenting line 11
Thx about explaination of how preview is working |
No description provided.