-
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
add task solution #2241
base: master
Are you sure you want to change the base?
add task solution #2241
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 efforts on the formatDate
function! While your code meets many of the task requirements, there are a few critical issues that need to be addressed to ensure it functions correctly across all scenarios.
-
Separator Handling: The assumption that the date separator is always at index 3 of the format arrays is not universally applicable. This could lead to incorrect date formatting for certain inputs. Consider implementing a more flexible approach to determine the separator dynamically.
-
Century Logic: The current logic for determining the century based on two-digit years is not robust. The assumptions made (e.g., years >= 30 belong to the 20th century) may not hold true for all cases and could lead to incorrect results. Revisiting this logic to ensure it accurately reflects the intended century would be beneficial.
-
Year Length Check: The condition checking if a year is a two-digit year using
splitDate[i] < 2000
is incorrect, assplitDate[i]
is a string. It would be more appropriate to check the length of the string or the value directly.
Addressing these issues will enhance the reliability and accuracy of your function. Keep up the great work, and I'm confident you'll nail it with these adjustments! 😊
✨ 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.
@@ -8,7 +8,47 @@ | |||
* @returns {string} | |||
*/ | |||
function formatDate(date, fromFormat, toFormat) { | |||
// write code here | |||
const splitDate = date.split(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 assumption that the date separator is always at index 3 of the fromFormat
array may not be valid for all date formats. Consider dynamically determining the separator based on the format string.
if ( | ||
newFromFormat[i] === 'YYYY' && | ||
newToFormat[x] === 'YY' && | ||
splitDate[i] < 2000 |
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 condition splitDate[i] < 2000
is incorrect for checking if a year is a two-digit year. This should be checking the length of the string or the value directly, as splitDate[i]
is a string.
if ( | ||
newFromFormat[i] === 'YY' && | ||
newToFormat[x] === 'YYYY' && | ||
splitDate[i] < 30 |
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 condition splitDate[i] < 30
assumes that any two-digit year less than 30 belongs to the 21st century. This logic might not be universally applicable and could lead to incorrect results for dates intended to be in the 20th century.
if ( | ||
newFromFormat[i] === 'YY' && | ||
newToFormat[x] === 'YYYY' && | ||
splitDate[i] >= 30 |
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 condition splitDate[i] >= 30
assumes that any two-digit year 30 or greater belongs to the 20th century. This logic might not be universally applicable and could lead to incorrect results for dates intended to be in the 21st century.
} | ||
} | ||
|
||
return newArr.join(newToFormat[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 use of newToFormat[3]
as the separator for joining the date components assumes the separator is at index 3, which may not be correct for all formats. Consider using a more flexible approach to determine the separator.
No description provided.