-
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 #2218
base: master
Are you sure you want to change the base?
Solution #2218
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 the formatDate
function! While your solution meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the logic for determining the separator in both fromFormat
and toFormat
is incorrect, as it assumes the separator is the last element of the array. Additionally, the handling of two-digit years needs adjustment to ensure the correct order of operations. Please address these issues to ensure the function works as intended. Keep up the great work, and I'm confident you'll have this resolved in no time! 😊
✨ 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 dateParts = {}; | |||
const separator = fromFormat[fromFormat.length - 1]; |
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 separator is incorrect. The separator is not necessarily the last element of the fromFormat
array. You should consider a different approach to identify the separator used in the date
string.
src/formatDate.js
Outdated
for (let i = 0; i < fromFormat.length; i++) { | ||
if (fromFormat[i] === 'YY') { | ||
dateParts['YYYY'] = | ||
+splitDate[i] < 30 ? `20${splitDate[i]}` : `19${splitDate[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 logic for handling two-digit years is incorrect. The expression +splitDate[i] < 30
will not work as intended because the unary plus operator has higher precedence than the addition operator. Consider using parentheses to ensure the correct order of operations.
src/formatDate.js
Outdated
dateParts['YY'] = dateParts['YYYY'].slice(-2); | ||
} | ||
|
||
const newSeparator = toFormat[toFormat.length - 1]; |
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
, the logic for determining the separator in toFormat
is incorrect. The separator is not necessarily the last element of the toFormat
array.
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 formatDate
function! 🎉 Your solution meets the task requirements and constraints, effectively identifying the separator from the fromFormat
array and handling the conversion between two-digit and four-digit years. However, there are some critical issues that need attention:
- Separator Logic: The current logic assumes the first non-date part is the separator, which might not be accurate if multiple separators exist. Consider using a regular expression to extract the separator from the date string to improve accuracy.
- Final Return Statement: The
slice(0, -1)
call ontoFormat
is incorrect as it removes the last element of the array, potentially omitting a necessary part of the format. Ensure all parts oftoFormat
are included in the final result.
These improvements will enhance the functionality of your code. Remember, addressing these issues is crucial for future tasks to avoid similar problems. Keep up the great work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
const separator = fromFormat.find( | ||
(part) => | ||
part !== 'DD' && part !== 'MM' && part !== 'YYYY' && part !== '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 separator is incorrect. It assumes that the first non-date part is the separator, but this might not be the case if multiple separators exist. Consider using a regular expression to extract the separator from the date string.
const newSeparator = toFormat.find( | ||
(part) => | ||
part !== 'DD' && part !== 'MM' && part !== 'YYYY' && part !== '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 new separator is incorrect for the same reason as the fromFormat
separator. Ensure that you correctly identify the separator used in the toFormat
.
.slice(0, -1) | ||
.map((part) => dateParts[part]) | ||
.join(newSeparator); |
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 slice(0, -1)
call on toFormat
is incorrect as it removes the last element of the array, which might be a necessary part of the format. Ensure that all parts of toFormat
are included in the final result.
No description provided.