Date-format#2675
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is one critical behavior that conflicts with the explicit requirements and the intended use of the format arrays. In src/formatDate.js, your separator handling both (1) tries to infer the separator by scanning the date string and (2) includes a sep === undefined / "Wrong separator!" path; this is effectively input validation, which the task explicitly says not to do, and it ignores that the separator is defined directly in fromFormat[fromFormat.length - 1]. To align with the requirements, you should remove the validation/error branch entirely and simply set the separator as const sep = fromFormat[fromFormat.length - 1];, then split/join using that value instead of inspecting characters in the input date. This change will also fully satisfy the checklist items that say the format arrays themselves describe the separator and that we should assume correct input.
A smaller, non-blocking improvement: when expanding YY to YYYY, your line obj['YYYY'] = obj['YY'] < 30 ? '20' + obj['YY'] : '19' + obj['YY']; relies on implicit string-to-number coercion; consider writing Number(obj['YY']) < 30 to make the numeric intent explicit and avoid subtle type issues. Overall, your handling of reordering, separator changes, and year-length conversion is solid; once you simplify the separator logic and remove the validation branch, this will cleanly meet all stated requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| } | ||
| } | ||
|
|
||
| if (sep === undefined) { |
There was a problem hiding this comment.
sep is initialized to an empty string and then later checked against undefined, which can never be true here; this also adds unnecessary validation despite the task stating there is no need to validate input. Consider removing this check and deriving the separator directly from fromFormat instead of scanning date.
| for (const char of date) { | ||
| if (char === fromFormat[fromFormat.length - 1]) { |
There was a problem hiding this comment.
The separator is inferred from the characters in date and compared to fromFormat[fromFormat.length - 1], but the task states that the format arrays themselves describe the separator. It would be clearer and more reliable to use const sep = fromFormat[fromFormat.length - 1]; instead of inspecting the input string.
| } | ||
|
|
||
| if ('YY' in obj) { | ||
| obj['YYYY'] = obj['YY'] < 30 ? '20' + obj['YY'] : '19' + obj['YY']; |
There was a problem hiding this comment.
obj['YYYY'] = obj['YY'] < 30 ? '20' + obj['YY'] : '19' + obj['YY']; relies on implicit numeric coercion of the string obj['YY']. Given assumptions about valid input this likely works, but making the comparison numeric explicitly would better reflect the requirement YY < 30 and avoid subtle type issues.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it correctly implements all the required behavior for formatDate: parsing based on fromFormat, reordering according to toFormat, changing separators, and handling both YYYY → YY and YY → YYYY conversions with the specified 20xx/19xx rule. Functionally, your code now satisfies the task requirements and should work for the expected valid inputs.
The only remaining point is a minor style/clarity improvement: in the YY to YYYY conversion logic (where you compare obj['YY'] < 30), you are relying on implicit string-to-number coercion. While this works given the assumptions, it is clearer and safer to write something like Number(obj['YY']) < 30 so future readers immediately see the numeric intent. This is not a blocker, but it’s a good habit to avoid subtle type issues in JavaScript. Overall, well done on aligning the implementation closely with the specification and refining it based on earlier feedback.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| if ('YY' in obj) { | ||
| obj['YYYY'] = obj['YY'] < 30 ? '20' + obj['YY'] : '19' + obj['YY']; |
There was a problem hiding this comment.
When converting YY to YYYY, this comparison relies on implicit string-to-number coercion; consider using Number(obj['YY']) < 30 to make the numeric intent explicit and avoid subtle type issues.
No description provided.