-
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
formatDate function v1.1 #2135
base: master
Are you sure you want to change the base?
formatDate function v1.1 #2135
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 job on implementing the core functionality! 🎉 While there are some minor areas for improvement, such as ensuring consistent application of logic and handling separators more flexibly, these don't critically impact the functionality. Keep up the good work and consider these suggestions for future enhancements!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
function formatDate(date, fromFormat, toFormat) { | ||
// write code here | ||
const prevSplitter = fromFormat[3]; |
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 at the third index of the format string. This might not be true for all formats. Consider using a more reliable method to determine the separator.
function formatDate(date, fromFormat, toFormat) { | ||
// write code here | ||
const prevSplitter = fromFormat[3]; | ||
const newSplitter = toFormat[3]; |
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 previous comment, this line assumes the separator is at the third index of the format string. Ensure that the separator is correctly identified for all possible formats.
setTheDate('DD'); | ||
setTheDate('MM'); |
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 function calls for setting the date types 'DD' and 'MM' are correct, but ensure that the order of these calls matches the order in the 'toFormat' array.
if (fromFormat.includes('YYYY') && toFormat.includes('YY')) { | ||
setTheDate('YYYY', 'YY'); | ||
|
||
newRawDate[toFormat.indexOf('YY')] = newRawDate[ | ||
toFormat.indexOf('YY') | ||
].slice(2, 4); | ||
} |
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 converting from 'YYYY' to 'YY', the code correctly slices the last two digits. Ensure that this logic is applied consistently across all relevant parts of the code.
if (fromFormat.includes('YY') && toFormat.includes('YYYY')) { | ||
if (rawDate[fromFormat.indexOf('YY')] < 30) { | ||
newRawDate[toFormat.indexOf('YYYY')] = | ||
`20${rawDate[fromFormat.indexOf('YY')]}`; | ||
} | ||
|
||
if (rawDate[fromFormat.indexOf('YY')] >= 30) { | ||
newRawDate[toFormat.indexOf('YYYY')] = | ||
`19${rawDate[fromFormat.indexOf('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 converting 'YY' to 'YYYY' is implemented correctly, but ensure that the condition for determining the century (using 20 or 19) is consistently applied and tested with edge cases.
No description provided.