-
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 #2002
base: master
Are you sure you want to change the base?
Solution #2002
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🔥!
I like the idea of using format designations as map keys, good thinking there.
fromFormat.slice(0, 3).forEach((part, index) => { | ||
dateMap[part] = dateParts[index]; |
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.
Ensure that the date parts are correctly mapped even if the fromFormat
array is not in the order of 'YYYY', 'MM', 'DD'. The current implementation assumes a specific order, which may not always be the case.
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.
done
src/formatDate.js
Outdated
if (dateMap['YYYY'] && toFormat.includes('YY')) { | ||
dateMap['YY'] = dateMap['YYYY'].slice(2); | ||
} else if (dateMap['YY'] && toFormat.includes('YYYY')) { | ||
dateMap['YYYY'] = | ||
dateMap['YY'] < 30 ? '20' + dateMap['YY'] : '19' + dateMap['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.
When checking for the presence of 'YYYY' or 'YY' in dateMap
and deciding to convert between them, you should also verify that the corresponding format actually exists in fromFormat
. Otherwise, you may end up referencing a format that was never in the original date string.
src/formatDate.js
Outdated
if (dateMap['YYYY'] && toFormat.includes('YY')) { | ||
dateMap['YY'] = dateMap['YYYY'].slice(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.
When slicing the 'YYYY' to get 'YY', consider adding a check to ensure 'YYYY' is actually 4 characters long to avoid potential errors.
src/formatDate.js
Outdated
dateMap['YY'] = dateMap['YYYY'].slice(2); | ||
} else if (dateMap['YY'] && toFormat.includes('YYYY')) { | ||
dateMap['YYYY'] = | ||
dateMap['YY'] < 30 ? '20' + dateMap['YY'] : '19' + dateMap['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 conversion from 'YY' to 'YYYY' assumes that 'YY' will always be a number and does not handle cases where 'YY' could be malformed or non-numeric. It would be safer to parse the 'YY' part as an integer and then perform the comparison and concatenation.
dateMap['YY'] < 30 ? '20' + dateMap['YY'] : '19' + dateMap['YY']; | ||
} | ||
|
||
const newDate = 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 mapping from dateMap
to the new date format assumes that all required parts are present in dateMap
. Consider adding a check to ensure that each required part of toFormat
is available in dateMap
before attempting to map it to avoid undefined values in the output.
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.
GJ 👍
No description provided.