-
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
created SalaryInfo() methods, solved task #1351
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.
You've done a great job so far! 👍 However, there are a few improvements that can be made to enhance your code. Consider using System.lineSeparator()
for cross-platform compatibility and StringBuilder
for efficiency. Also, try giving more descriptive names to your variables. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
public String getSalaryInfo(String[] names, String[] data, | ||
String dateFrom, String dateTo) { | ||
String result = new String(); |
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 created a String object result
and then reassign it in a loop. This can lead to a large number of unused objects. It's better to use StringBuilder
for this purpose. The StringBuilder
object report
you have created is perfect for this.
|
||
} | ||
} | ||
result = report.append("\r\n").append(name).append(" - ").append(salary).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.
Here you're using \r\n
as a line separator. It's better to use System.lineSeparator()
to make your code cross-platform.
public class SalaryInfo { | ||
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
public 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.
Good job on making the DateTimeFormatter
a constant. This is a good practice as it enhances readability and maintainability of your code.
for (String name : names) { | ||
int salary = 0; | ||
for (String line: data) { | ||
String[] array = line.split(" "); | ||
LocalDate localDate = LocalDate.parse(array[0], formatter); | ||
if ((localDate.isAfter(localDateFrom) || localDate.equals(localDateFrom)) | ||
&& (localDate.isBefore(localDateTo) | ||
|| localDate.equals(localDateTo)) && array[1].equals(name)) { | ||
salary += Integer.parseInt(array[2]) | ||
* Integer.parseInt(array[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.
Well done on using nested loops to calculate the salary. This is an effective way to solve the problem.
String[] array = line.split(" "); | ||
LocalDate localDate = LocalDate.parse(array[0], formatter); | ||
if ((localDate.isAfter(localDateFrom) || localDate.equals(localDateFrom)) | ||
&& (localDate.isBefore(localDateTo) | ||
|| localDate.equals(localDateTo)) && array[1].equals(name)) { | ||
salary += Integer.parseInt(array[2]) | ||
* Integer.parseInt(array[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.
You have used split
method to divide data and then parse it to the required types. This is a good approach. However, consider giving a more descriptive name to the array
variable. For example, dataParts
or dataElements
would give a clearer indication of what it contains.
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 work on the task! 😊 Your code is already functional and efficient. However, there are a few minor areas for improvement, such as variable naming and code readability. These don't impact the functionality, so I'm approving your pull request. Keep these suggestions in mind for your future tasks. Keep it up! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
public class SalaryInfo { | ||
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
public 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.
The constant formatter
should be in uppercase, as per the Java naming conventions for constants. It should look like this: public static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("dd.MM.yyyy");
return null; | ||
public static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); | ||
|
||
public 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.
The method name getSalaryInfo
is not very descriptive. Consider renaming it to something more informative, such as calculateSalariesForPeriod
.
String[] dataParts = line.split(" "); | ||
LocalDate localDate = LocalDate.parse(dataParts[0], 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.
The variable dataParts
is not very descriptive. Consider renaming it to something more informative, such as lineParts
or parsedLine
.
if ((localDate.isAfter(localDateFrom) || localDate.equals(localDateFrom)) | ||
&& (localDate.isBefore(localDateTo) | ||
|| localDate.equals(localDateTo)) && dataParts[1].equals(name)) { |
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 condition is a bit complex and hard to read. Consider breaking it down into smaller conditions and using descriptive variable names for them. This will make your code easier to understand.
salary += Integer.parseInt(dataParts[2]) | ||
* Integer.parseInt(dataParts[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.
These magic numbers 2
and 3
should be declared as constants, as per the checklist. Declare them as constants at the beginning of your class and give them descriptive names.
No description provided.