-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
solution #2160
base: master
Are you sure you want to change the base?
solution #2160
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.
Thank you for your hard work on this Pull Request! 😊 While your solution meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve it:
-
Index Validation: The use of
indexOf
can lead to an error iftoFormat[i]
is not found infromFormat
, resulting in a-1
index. Please add a check to ensure that theposition
is valid before accessingdateArray[position]
. -
Century Logic: The current logic for determining the century based on the year being less than 30 is not flexible for all scenarios. It would be beneficial to make this logic configurable or to provide a more robust solution to handle different use cases.
-
Separator Assumption: The code assumes that the separator is always the fourth element in the format arrays, which might not hold true if the format arrays are not structured as expected. Please add validation to ensure the format arrays contain the expected elements.
Addressing these issues will enhance the robustness and flexibility of your code. Keep up the great work, and I look forward to your updated submission! 🚀
✨ 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.
src/formatDate.js
Outdated
const [, , , fromSeparator] = fromFormat; | ||
const [, , , toSeparator] = toFormat; |
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 code assumes that the separator is always the fourth element in the format arrays. This might not be the case if the format arrays are not structured as expected. Consider validating the format arrays to ensure they contain the expected elements.
src/formatDate.js
Outdated
if (dateArray[fromYearIndex] < 30) { | ||
splitArrayDate.unshift(20); | ||
} else { | ||
splitArrayDate.unshift(19); |
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 logic for determining the century (19 or 20) based on the year being less than 30 is a common practice but might not be suitable for all use cases. Consider making this logic configurable or providing a more robust solution.
} | ||
|
||
for (let i = 0; i < dateArray.length; i++) { | ||
const position = fromFormat.indexOf(toFormat[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.
The indexOf
method will return -1 if the toFormat[i]
is not found in fromFormat
, which will cause an error when accessing dateArray[position]
. Consider adding a check to ensure position
is valid before using it.
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 improving the code! 🎉 You've made significant progress by addressing some of the key issues:
-
Index Validation: You've added a check to ensure that the
position
is valid before accessingdateArray[position]
. This prevents errors from invalid indices, which is a crucial improvement. -
Century Logic: The logic for determining the century has been implemented correctly based on the task requirements. However, consider making this logic more flexible or configurable in the future to handle different scenarios.
-
Separator Assumption: The code correctly handles the separator dynamically by using
pop()
, ensuring it doesn't rely on a fixed position. This is a good approach, but make sure the format arrays are structured as expected or add validation to handle unexpected structures.
While there are still some critical issues that need attention, such as making the century logic more robust and ensuring the format arrays are validated, the decision to approve this Pull Request is made to help you move forward with your learning. Please review the comments and continue to improve your code. Keep up the great work and learn from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
if (dateArray[fromYearIndex] < 30) { | ||
splitArrayDate.unshift(20); | ||
} else if (dateArray[fromYearIndex] >= 30) { | ||
splitArrayDate.unshift(19); |
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 logic for determining the century is hardcoded to assume that years less than 30 belong to the 2000s and years 30 or greater belong to the 1900s. This logic might not be suitable for all scenarios. Consider making this logic configurable or more robust to handle different use cases.
const fromSeparator = fromFormat.pop(); | ||
const toSeparator = toFormat.pop(); |
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 code assumes that the separator is always the last element in the format arrays. Ensure that the format arrays are structured as expected, or add validation to handle unexpected structures.
No description provided.