Solution #2185
Solution #2185
Conversation
There was a problem hiding this comment.
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:
-
Separator Handling: The current logic assumes that separators are not part of the format strings, which might not hold true for all input cases. It's important to ensure that your function can handle separators correctly, regardless of their position in the format arrays.
-
Year Conversion Logic: The conversion from 'YY' to 'YYYY' assumes that years less than 30 belong to the 2000s, which might not be accurate for all scenarios. Consider making this logic configurable or implementing a more robust solution to handle different use cases.
-
Separator Determination: The assumption that the last element of the format arrays is the separator might not always be valid. Please revise this logic to ensure that separators are correctly identified and handled, even if they are not explicitly included as the last element in the format arrays.
Once these issues are addressed, your solution will be much more robust and ready for approval. Keep up the great work, and don't hesitate to reach out if you have any questions! 😊
✨ 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
| const fromSeparator = fromFormat[fromFormat.length - 1]; | ||
| const toSeparator = toFormat[toFormat.length - 1]; |
There was a problem hiding this comment.
The logic for determining the separators assumes that the last element of the format arrays is the separator. This might not be a valid assumption if the format arrays do not include the separator as their last element. Consider revising this logic to handle cases where the separator is not explicitly included in the format arrays.
| if (toFormat.includes('YYYY') && fromFormat.includes('YY')) { | ||
| year = year < 30 ? `20${year}` : `19${year}`; |
There was a problem hiding this comment.
The logic for converting 'YY' to 'YYYY' assumes that years less than 30 belong to the 2000s and others to the 1900s. This might not be accurate for all use cases. Consider making this logic configurable or providing a more robust solution.
src/formatDate.js
Outdated
| for (let i = 0; i < toFormat.length - 1; i++) { | ||
| if (toFormat[i] === 'YYYY' || toFormat[i] === 'YY') { | ||
| newDateParts.push(year); | ||
| } else if (toFormat[i] === 'MM') { | ||
| newDateParts.push(month); | ||
| } else if (toFormat[i] === 'DD') { | ||
| newDateParts.push(day); | ||
| } | ||
| } | ||
|
|
||
| return newDateParts.join(toSeparator); |
There was a problem hiding this comment.
The loop for constructing the new date parts assumes that the format arrays do not include the separator as part of the format strings. Ensure that this assumption holds true for all input cases or adjust the logic to handle separators correctly.
There was a problem hiding this comment.
Review Summary
Great effort on implementing the formatDate function! 🎉 While your code is on the right track, there are a few critical issues that need attention:
-
Separator Handling: The logic for determining separators in
fromFormatandtoFormatis flawed. The conditionarg !== 'YYYY' || arg !== 'YY' || arg !== 'MM' || arg !== 'DD'will always evaluate totrue. You should use&&instead of||to correctly identify non-date parts. Additionally, ensure that separators are correctly identified and handled, even if they are not explicitly included as the last element in the format arrays. -
Year Conversion Logic: The current logic assumes that years less than 30 belong to the 2000s. While this is a common approach, it might not be suitable for all scenarios. Consider making this logic more flexible or configurable to handle different cases.
Despite these issues, the decision is to APPROVE your Pull Request to help you move forward with your learning. Please review the comments and make the necessary improvements to your code. Addressing these critical issues will enhance the functionality and robustness of your solution. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| for (const arg of fromFormat) { | ||
| if (arg !== 'YYYY' || arg !== 'YY' || arg !== 'MM' || arg !== 'DD') { | ||
| fromSeparator = arg; | ||
| } |
There was a problem hiding this comment.
The condition arg !== 'YYYY' || arg !== 'YY' || arg !== 'MM' || arg !== 'DD' is incorrect because it will always evaluate to true. This is because arg cannot be equal to all these values at the same time. You should use && instead of || to check if arg is not equal to any of these values.
| if (toFormat.includes('YYYY') && fromFormat.includes('YY')) { | ||
| year = year < 30 ? `20${year}` : `19${year}`; |
There was a problem hiding this comment.
The logic for converting 'YY' to 'YYYY' assumes that years less than 30 belong to the 2000s. This assumption might not be valid for all scenarios. Consider making this logic configurable or more robust to handle different cases.
There was a problem hiding this comment.
This was also stated in the problem statement.
The function can change a separator, reorder the date parts of convert a year from 4 digits to 2 digits and back.
When converting from YYYY to YY just use 2 last digit (1997 -> 97).
When converting from YY to YYYY use 20YY if YY < 30 and 19YY otherwise.
| let toSeparator; | ||
|
|
||
| for (let i = 0; i < toFormat.length; i++) { | ||
| if (toFormat[i] === 'YYYY' || toFormat[i] === 'YY') { | ||
| newDateParts.push(year); | ||
| } else if (toFormat[i] === 'MM') { | ||
| newDateParts.push(month); | ||
| } else if (toFormat[i] === 'DD') { | ||
| newDateParts.push(day); | ||
| } else { | ||
| toSeparator = toFormat[i]; |
There was a problem hiding this comment.
The logic for determining the toSeparator assumes that the separator is not part of the format strings, which might not always be the case. Ensure that separators are correctly identified and handled, even if they are not explicitly included as the last element in the format arrays.
No description provided.