-
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
first solution #1297
base: master
Are you sure you want to change the base?
first solution #1297
Conversation
return null; | ||
StringBuilder getSalaryOutput = new StringBuilder(); | ||
int[] salary = new int[names.length]; | ||
final 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.
Since the formatter is used multiple times in the code, it should be made into a constant field. Define it as a private static final field at the class level and use an uppercase name to follow Java naming conventions for constants.
public class SalaryInfo { | ||
|
||
public String getSalaryInfo(String[] names, String[] data, 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 class and method implementation should not start with an empty line. Please remove the empty line at the beginning of the method implementation.
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
StringBuilder getSalaryOutput = 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.
The variable name 'getSalaryOutput' is not very informative. Consider renaming it to something like 'salaryReportBuilder' to better reflect its purpose.
.append(dateFrom) | ||
.append(" - ") | ||
.append(dateTo); | ||
for (String dataArray : 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.
Using 'dataArray' as a variable name is not informative. Consider renaming it to something like 'record' or 'dataEntry' to better reflect the data it represents.
if (localDate.isAfter(dateFromFormated) | ||
&& localDate.isBefore(dateToFormated) | ||
|| localDate.isEqual(dateFromFormated) | ||
|| localDate.isEqual(dateToFormated)) { |
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 condition to check if the date is within the range can be simplified by using !localDate.isBefore(dateFromFormated) && !localDate.isAfter(dateToFormated), which is more readable.
salary[j] = salary[j] | ||
+ (Integer.parseInt(dataSplit[2]) | ||
* Integer.parseInt(dataSplit[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.
Instead of recalculating the salary each time, consider using a local variable to store the parsed values of 'dataSplit[2]' and 'dataSplit[3]' before the multiplication to improve code readability.
} | ||
} | ||
} | ||
for (int i = 0; i < salary.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.
There are redundant loops here, the task can be solved with a maximum of two loops. Consider combining the logic to reduce the number of loops and increase efficiency.
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 implement changes required by AI mentor.
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.
Small corrections to be done.
@@ -5,40 +5,35 @@ | |||
|
|||
public class SalaryInfo { | |||
|
|||
private static final 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.
Constant variables should be all uppercase letters.
You can have a look here: https://mate-academy.github.io/style-guides/java/java.html#s5.2.4-constant-names
for (String dataEntry : data) { | ||
String[] dataSplit = dataEntry.split(" "); | ||
LocalDate localDate = LocalDate.parse(dataSplit[0], formatter); | ||
if (!localDate.isBefore(dateFromFormated) && !localDate.isAfter(dateToFormated)) { |
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.
I would extract the check in the if to a separate method for more readability and clarity, eg. isDataWithinRange()
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! There is a small mistake that you should consider, but I'm sure you can handle it, so approved!
LocalDate dateFromFormated = LocalDate.parse(dateFrom, formatter); | ||
LocalDate dateToFormated = LocalDate.parse(dateTo, 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.
You should keep these variables. Now when you are calling your new method (isDataWithinRange
) you are unnecessary creating 2 new variables on each method call. Rework your new method a bit, so that it accepts as parameters already formated dateFrom
and dateTo
.
No description provided.