-
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
salaryInfo done #1321
base: master
Are you sure you want to change the base?
salaryInfo done #1321
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.
Hey there! 👋 It looks like there are a few important areas that need some attention before we can move forward. The method names could be more informative, and it's essential to avoid using two-dimensional arrays as suggested in the checklist. Also, let's make sure to use constants instead of magic numbers to make the code clearer and more maintainable. Keep up the great work, and with a bit of tweaking, your code will be in top shape! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -0,0 +1,21 @@ | |||
package core.basesyntax; | |||
|
|||
public class Main { |
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. Please remove the empty line at the start of the class definition.
|
||
public class Main { | ||
public static void main(String[] args) { | ||
String dateFrom = "01.04.2019"; |
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. Consider defining 'dateFrom' and 'dateTo' as constant fields if they are not meant to be changed.
public class Main { | ||
public static void main(String[] args) { | ||
String dateFrom = "01.04.2019"; | ||
String dateTo = "30.04.2019"; |
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. Consider defining 'dateFrom' and 'dateTo' as constant fields if they are not meant to be changed.
public static void main(String[] args) { | ||
String dateFrom = "01.04.2019"; | ||
String dateTo = "30.04.2019"; | ||
String[] names = new String[] {"John", "Andrew", "Kate"}; |
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 the variables and methods. The variable 'names' could be more descriptive, like 'employeeNames'.
public class SalaryInfo { | ||
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
String[][] salaryByName = new String[names.length][2]; |
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 as it's mentioned in the checklist. Instead, consider creating a class to represent the employee's salary details or use a different approach to store this information.
salaryByName[i][0] = names[i]; | ||
salaryByName[i][1] = "0"; | ||
} | ||
String[][] splitedData = splitData(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.
The use of two-dimension arrays is discouraged as per the checklist. Try to find an alternative way to process the data without using two-dimension arrays.
} | ||
|
||
private boolean dayCheck(String dateToCheck, String dateFrom, String dateTo) { | ||
DateTimeFormatter formatter = 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.
As per the checklist, if you create a formatter, make it a constant field. This allows for better reusability and maintainability of the code.
if (dayCheck(splitedData[i][0], dateFrom, dateTo)) { | ||
for (int j = 0; j < names.length; j++) { | ||
if (splitedData[i][1].equals(salaryByName[j][0])) { | ||
try { | ||
salaryByName[j][1] = | ||
String.valueOf((Integer.parseInt(salaryByName[j][1]) | ||
+ (Integer.parseInt(splitedData[i][2]) | ||
* Integer.parseInt(splitedData[i][3])))); | ||
} catch (NumberFormatException ex) { | ||
System.out.println("Error parsing string to number: " | ||
+ ex.getMessage()); | ||
} | ||
} | ||
} | ||
} |
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 create redundant loops. Two loops are enough to solve this task. Try to optimize the code to use only two loops.
private String getRezultToString(String[][] salaryByName, String dateFrom, String dateTo) { | ||
StringBuilder rezultMessage = new StringBuilder(); | ||
rezultMessage.append("Report for period ").append(dateFrom).append(" - ") | ||
.append(dateTo).append(System.lineSeparator()); | ||
for (int i = 0; i < salaryByName.length; i++) { | ||
rezultMessage.append(salaryByName[i][0]).append(" - ") | ||
.append(salaryByName[i][1]).append(System.lineSeparator()); | ||
} | ||
return rezultMessage.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.
The method name 'getRezultToString' has a typo and is not very informative. Consider renaming it to accurately reflect its purpose, such as 'formatSalaryReport'.
return rezultMessage.toString(); | ||
} | ||
|
||
private boolean dayCheck(String dateToCheck, String dateFrom, String 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.
The method name 'dayCheck' is not informative. Consider renaming it to something more descriptive, like 'isDateWithinRange'.
for (int i = 0; i < names.length; i++) { | ||
emploeesSalaries[i] = new EmploeesSalary(); | ||
emploeesSalaries[i].setEmploeesName(names[i]); | ||
emploeesSalaries[i].setEarnedSalary(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.
all magic numbers should be constant, check all
if (i < emploeesSalaries.length - 1) { | ||
resultMessage.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 hack
just add lineSeparator at the beginning, not the end
if ((dateToCheck.isAfter(dateFrom) && dateToCheck.isBefore(dateTo)) | ||
|| (dateToCheck.isEqual(dateFrom) || dateToCheck.isEqual(dateTo))) { | ||
return true; | ||
} else { | ||
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.
instead of
if boolean return true else return false
just write
return boolean
@@ -0,0 +1,22 @@ | |||
package core.basesyntax; | |||
|
|||
public class EmploeesSalary { |
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 class EmploeesSalary { | |
public class Employee { |
do you really need two model classes?
No description provided.