-
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 #2170
base: master
Are you sure you want to change the base?
Solution #2170
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 project! 🎉 While your solution meets the task requirements in many ways, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Century Logic for 'YY': The current logic for determining the century for two-digit years ('YY') assumes that any year less than '30' belongs to the 21st century. This assumption might not hold true in all contexts, and it's important to ensure that this logic aligns with the expected behavior for your application.
-
Array Mutation with
pop()
: The use ofpop()
on bothfromFormat
andtoFormat
modifies the original arrays. This could lead to unintended side effects if these arrays are used elsewhere in the code. Consider using non-mutating methods likeslice()
or the spread operator to avoid altering the input arrays.
Addressing these issues will enhance the robustness and reliability of your function. Keep up the great work, and I look forward to seeing your improvements! 😊
✨ 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
@@ -8,7 +8,29 @@ | |||
* @returns {string} | |||
*/ | |||
function formatDate(date, fromFormat, toFormat) { | |||
// write code here | |||
const separator = fromFormat.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 use of pop()
on fromFormat
modifies the original array, which might not be intended if fromFormat
is used elsewhere. Consider using slice()
or spread operator
to avoid mutating the input array.
src/formatDate.js
Outdated
dateParts['YY'] < '30' ? '20' + dateParts['YY'] : '19' + dateParts['YY']; | ||
} | ||
|
||
const newSeparator = 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.
Similar to the issue with fromFormat
, using pop()
on toFormat
modifies the original array. Consider using slice()
or spread operator
to avoid mutating the input array.
dateParts['YYYY'] = | ||
dateParts['YY'] < '30' ? '20' + dateParts['YY'] : '19' + dateParts['YY']; |
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 for 'YY' might not be accurate for all cases. It assumes that any 'YY' less than '30' belongs to the 21st century, which might not be correct depending on the context. Ensure this logic aligns with the expected behavior.
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 by using non-mutating methods for handling format arrays! 🎉 The function formatDate
effectively meets the task requirements by converting date formats, changing separators, reordering date parts, and handling year conversions between two-digit and four-digit formats. The logic for converting from YY
to YYYY
is implemented as specified: using 20YY
if YY < 30
and 19YY
otherwise.
However, it's important to note that the logic for determining the century for two-digit years assumes that any year less than '30' belongs to the 21st century. This assumption might not be valid in all contexts, so please consider revising this logic to ensure it aligns with the expected behavior for your application.
Overall, your code adheres to the constraints provided, and the issues from the previous review have been addressed. Keep up the good work, and remember to review the comments to further improve your code. Learning from these insights will help you enhance your skills and avoid similar issues in the future. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
if (toFormatCopy.includes('YYYY') && dateParts['YY']) { | ||
dateParts['YYYY'] = | ||
dateParts['YY'] < '30' ? '20' + dateParts['YY'] : '19' + dateParts['YY']; |
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 for two-digit years ('YY') assumes that any year less than '30' belongs to the 21st century. This assumption might not be valid in all contexts. Consider revising this logic to ensure it aligns with the expected behavior for your application.
No description provided.