-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Homework for getiing thw sallary info #1382
base: master
Are you sure you want to change the base?
Conversation
… given period of time accordingly to quantity of working hours and hourly rate
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.
Thank you for your hard work on this pull request! 😊 While you've made some good progress, there are a few critical issues that need to be addressed before we can approve the changes:
-
Date Handling: The current logic for date comparison is flawed. It checks if the record date is less than or equal to the start date and greater than or equal to the end date, which is incorrect. The correct logic should ensure that the record date is between the start date and end date inclusively. This is a critical issue as it directly impacts the functionality of the
getSalaryInfo
method. Please consider usingLocalDate
for more accurate and reliable date comparisons. -
Syntax Error: There's a missing semicolon at the end of a statement. This is a syntax error that needs to be fixed to ensure the code compiles correctly.
Once these issues are resolved, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you need any help. You're almost there! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
@@ -2,6 +2,48 @@ | |||
|
|||
public class SalaryInfo { | |||
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | |||
return null; | |||
StringBuilder outputString = new StringBuilder("Report for period ") |
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 semicolon is missing at the end of the statement. Ensure to terminate statements with a semicolon.
if ((Integer.parseInt(splittedRecordDate[2]) | ||
<= Integer.parseInt(splittedDateFrom[2]) | ||
&& (Integer.parseInt(splittedRecordDate[1]) | ||
<= Integer.parseInt(splittedDateFrom[1]) | ||
&& (Integer.parseInt(splittedRecordDate[0]) | ||
<= Integer.parseInt(splittedDateFrom[0])))) | ||
|| (Integer.parseInt(splittedRecordDate[2]) | ||
>= Integer.parseInt(splittedDateTo[2]) | ||
&& (Integer.parseInt(splittedRecordDate[1]) | ||
>= Integer.parseInt(splittedDateTo[1]) | ||
&& (Integer.parseInt(splittedRecordDate[0]) | ||
> Integer.parseInt(splittedDateTo[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.
The date comparison logic is incorrect. The current logic checks if the record date is less than or equal to the start date and greater than or equal to the end date, which is logically flawed. You should check if the record date is between the start date and end date inclusively. Consider using a more straightforward date comparison approach or a date library to handle this.
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.
Review Summary
Great job on addressing the previous feedback and improving the code in SalaryInfo.java
! 🎉
Key Improvements:
- Date Handling: You've successfully corrected the logic for date comparison. The use of
LocalDate
ensures accurate and inclusive checks between the start and end dates. - Syntax Error: The missing semicolon issue has been resolved, and the code now compiles without syntax errors.
Your solution now meets the task requirements and constraints. However, it's important to always double-check for syntax errors, like missing semicolons, to ensure smooth compilation.
Moving Forward:
While the current code is approved, remember to review the comments and address any minor issues to enhance your coding skills. This will help you avoid similar problems in the future and improve the overall quality of your code.
Keep up the great work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
LocalDate ldRecordDate = LocalDate.parse(splittedEmployeeRecord[0], FORMATTER); | ||
if ((ldRecordDate.equals(ldDateFrom) || ldRecordDate.isAfter(ldDateFrom)) | ||
&& (ldRecordDate.equals(ldDateTo) || ldRecordDate.isBefore(ldDateTo))) { | ||
totalEmployeeSalary += Integer.parseInt(splittedEmployeeRecord[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.
There is a missing semicolon at the end of this line. This is a syntax error that needs to be fixed to ensure the code compiles correctly.
implemented getSalaryInfo() method to calculate employee's salary for given period of time accordingly to quantity of working hours and hourly rate