-
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
tests fails cuz of stupid separator #1310
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.
Great job on your Pull Request! There are a few minor suggestions for improvement, but none of them are critical issues. Remember to consider using StringBuilder
for string concatenation in loops, System.lineSeparator()
for platform-independent line breaks, and defining frequently used objects like formatters as constants for better code readability. Keep up the good work! 😊👍
@@ -1,7 +1,32 @@ | |||
package core.basesyntax; | |||
import java.time.LocalDate; | |||
import java.time.format.DateTimeFormatter; | |||
|
|||
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 method implementation should not begin with an empty line. Please remove the empty line at the start of the method.
|
||
public class SalaryInfo { | ||
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
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 created here and used multiple times, it should be made a constant field. This will make your code easier to understand and maintain.
} | ||
} | ||
} | ||
report.append("\n").append(name).append(" - ").append(Integer.toString(salary)); |
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 using '\n' for line breaks, use System.lineSeparator()
to ensure your code is platform-independent.
} | ||
} | ||
} | ||
report.append("\n").append(name).append(" - ").append(Integer.toString(salary)); |
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.
It's better to use StringBuilder
for appending strings in a loop rather than using +
or Integer.toString()
. This is because StringBuilder
is more efficient as it doesn't create new objects for each concatenation.
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 :)
Please apply changes from ai-mentor
String[] parts = line.split(" "); | ||
if (parts[1].equals(name)) { | ||
LocalDate workDate = LocalDate.parse(parts[0], formatter); | ||
if (workDate.isAfter(startDate.minusDays(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.
You could use compareTo to compare dates directly. This is more straightforward and reduces the risk of off-by-one errors with minusDays and plusDays. e.g
if (workDate.compareTo(startDate) >= 0 && workDate.compareTo(endDate) <= 0)
I would also extract the check to a method e.g. 'isDateWithinRange'
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 consider the comment
public class SalaryInfo { | ||
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
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.
String[] parts = line.split(" "); | ||
if (parts[1].equals(name)) { | ||
LocalDate workDate = LocalDate.parse(parts[0], formatter); | ||
if (workDate.isAfter(startDate.minusDays(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.
Please consider the comment
If you are interested please have a look on the article about creating commits: https://cbea.ms/git-commit/ |
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! Please read the comments and implement them. Also, you can create constant fields for specific indexes, instead of hardcoding values like '1' or '0'. For example:
private static final int INDEX_OF_WORKING_HOURS = 2;
private static final int INDEX_OF_INCOME_PER_HOUR = 3;
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.
public class SalaryInfo { | |
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | |
public class SalaryInfo { | |
private static final DateTimeFormatter DATE_TIME_FORMATTER = | |
DateTimeFormatter.ofPattern("dd.MM.yyyy"); | |
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { |
public class SalaryInfo { | ||
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
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.
final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
String[] parts = line.split(" "); | ||
if (parts[1].equals(name)) { | ||
LocalDate workDate = LocalDate.parse(parts[0], formatter); | ||
if (workDate.compareTo(startDate) >= 0 && workDate.compareTo(endDate) <= 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.
As Beata mentioned, it would be more readable and clean if you would extract this check into a separate method.
its allways an exception when trying to complite tests, cuz of "separator???"