done #1393
done #1393
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this Pull Request! 😊 While your solution meets most of the task requirements, there are a few critical issues that need to be addressed before we can approve it:
-
Date Parsing Exception Handling: Throwing an
ArrayIndexOutOfBoundsExceptionfor a wrong date format is not appropriate. Consider using a more descriptive exception likeIllegalArgumentExceptionto indicate an invalid input format. This will make your code more robust and easier to debug. -
Regex Pattern for Splitting Dates: The regex pattern
split("[\\.]")is incorrect for splitting by dot. It should besplit("\\.")to correctly split the date string by periods. This change is necessary to ensure that the date parsing works as expected.
Please address these issues to improve the functionality and clarity of your code. Once these changes are made, we can proceed with approving your Pull Request. Keep up the great work! 🚀
✨ 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.
| } | ||
|
|
||
| public LocalDate parseDate(String date) { | ||
| String[] parsedDate = date.split("[\\\\.]"); |
There was a problem hiding this comment.
The regex pattern in split("[\\.]") is incorrect for splitting by dot. It should be split("\\.") to correctly split the date string by periods.
| throw new ArrayIndexOutOfBoundsException("Wrong date format. Got: " | ||
| + date + " Should be dd.mm.yyyy format."); |
There was a problem hiding this comment.
Throwing ArrayIndexOutOfBoundsException for a wrong date format is not appropriate. Consider using a more descriptive exception like IllegalArgumentException to indicate an invalid input format.
There was a problem hiding this comment.
Great job on your implementation! 🎉 Your code meets the task requirements and constraints effectively. Here are some key points from the review:
- The
EmployeeandMainclasses are implemented correctly with no critical issues. - The
SalaryInfoclass is mostly well-implemented, but there are a few critical issues to address:- Date Parsing Exception Handling: You've improved the code by correctly throwing an
IllegalArgumentExceptionfor wrong date formats. This is a positive change from the previousArrayIndexOutOfBoundsException. - Regex Pattern for Splitting Dates: The use of
split("\\.")is correct and resolves previous issues.
- Date Parsing Exception Handling: You've improved the code by correctly throwing an
Overall, your code aligns well with the task description and checklist. However, ensure that exception messages are clear and informative for better debugging.
Keep up the good work, and remember to address these issues in future projects to enhance your coding skills. Your effort and progress are commendable, and this approval is to encourage you to continue learning and improving! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| package core.basesyntax; | ||
|
|
||
| public class Main { | ||
| public static void main(String[] args) { |
There was a problem hiding this comment.
You use the main class to test your solution. But don’t push it to GitHub! Let’s remove this class
| public LocalDate parseDate(String date) { | ||
| String[] parsedDate = date.split("\\."); | ||
| if (parsedDate.length != 3) { | ||
| throw new IllegalArgumentException("Wrong date format. Got: " | ||
| + date + " Should be dd.mm.yyyy format."); | ||
| } | ||
| int day = Integer.parseInt(parsedDate[0]); | ||
| int month = Integer.parseInt(parsedDate[1]); | ||
| int year = Integer.parseInt(parsedDate[2]); | ||
| return LocalDate.of(year, month, day); | ||
| } |
There was a problem hiding this comment.
Let’s replace it using DateTimeFormatter (make it a constant field)
| return "Report for period " + (dateFrom) + (" - ") + (dateTo) + result; | ||
| } | ||
|
|
||
| public boolean isDateInRange(String date, String dateFrom, String dateTo) { |
There was a problem hiding this comment.
this method should be private
| } catch (NumberFormatException e) { | ||
| System.out.println("Invalid number format at index " + i); | ||
| } catch (DateTimeException e1) { | ||
| System.out.println("Invalid date format at index " + i + ". Should be dd.mm.yyyy"); | ||
| } |
There was a problem hiding this comment.
remove the try-catch block. You don’t have any specific logic for these exceptions.
| result.append(System.lineSeparator()).append(names[i]).append(" - ").append(salary); | ||
| totalSalary += employeeSalaries[i]; | ||
| } | ||
| return "Report for period " + (dateFrom) + (" - ") + (dateTo) + result; |
There was a problem hiding this comment.
use constants for literals and magic numbers
| return null; | ||
| int[] employeeSalaries = new int[names.length]; | ||
|
|
||
| for (int i = 0; i < data.length; i++) { |
There was a problem hiding this comment.
two loops are enough for this task, but you have three (let’s fix it)
| employee.setName(infoFromData[1]); | ||
|
|
||
| try { | ||
| if (infoFromData.length >= 4) { |
There was a problem hiding this comment.
you don’t need this check, so let’s remove it.
No description provided.