solution#2677
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current implementation mutates input parameters, which can lead to incorrect behavior for callers even though the main formatting logic is otherwise correct. In src/formatDate.js, using toFormat.pop() and fromFormat.pop() (likely near the top of the function where you extract separators) alters the original arrays passed by the caller; this violates the expectation that function inputs are not modified and can break code that reuses those arrays after calling formatDate.
To fix this, you can either work with copies of the arrays (e.g., const localToFormat = toFormat.slice() and const separator = localToFormat.pop()) or, even simpler, access the last element by index without mutating: const separator = toFormat[toFormat.length - 1]. The same approach should be applied to fromFormat so that you never call .pop() on the original arrays. Once that mutation issue is resolved, the rest of your implementation—handling separators, reordering parts, and converting between YY and YYYY with the correct rules—looks aligned with the requirements. You’re very close; addressing this input-mutation problem will make your solution both correct and safer to use in real code.
✨ 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
| */ | ||
| function formatDate(date, fromFormat, toFormat) { | ||
| // write code here | ||
| const newSeparator = toFormat.pop(); |
There was a problem hiding this comment.
Using toFormat.pop() mutates the array that is passed into the function. This can lead to unexpected behavior for callers that reuse toFormat later; consider working with a copy or accessing the last element without modifying the original array.
| function formatDate(date, fromFormat, toFormat) { | ||
| // write code here | ||
| const newSeparator = toFormat.pop(); | ||
| const oldSeparator = fromFormat.pop(); |
There was a problem hiding this comment.
Similarly, fromFormat.pop() mutates the input array. You can avoid this by copying the array first (e.g. with slice) or by reading the last element by index instead of popping it.
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 formatDate behavior: it respects fromFormat/toFormat (including their trailing separator entries), reorders date parts, converts between YYYY and YY with the specified rules, and no longer mutates the format arrays. The only remaining issue is technical clarity rather than a functional blocker: on the lines where you derive oldSeparator and newSeparator (e.g., using toFormat.slice(-1) and fromFormat.slice(-1)), you end up with one-element arrays like ['-'] instead of plain strings. JavaScript will coerce those arrays to strings when you call split and join, so it works today, but it’s less clear to readers and slightly brittle; a clearer approach would be to access the last element directly, e.g. const newSeparator = toFormat[toFormat.length - 1]; and const oldSeparator = fromFormat[fromFormat.length - 1];. Since this does not break functionality or violate any core requirement, it doesn’t block acceptance, but I recommend making that small cleanup for better readability and robustness. Overall, nice work handling the year conversion rules and avoiding mutation of the format arrays; your implementation satisfies the task’s functional requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| function formatDate(date, fromFormat, toFormat) { | ||
| // write code here | ||
| const newOrder = toFormat.slice(0, -1); | ||
| const newSeparator = toFormat.slice(-1); |
There was a problem hiding this comment.
toFormat.slice(-1) returns an array containing the last element (e.g. ['-']), not the separator string itself. Using that array in join relies on implicit coercion; consider taking the actual string value, e.g. const newSeparator = toFormat[toFormat.length - 1]; or toFormat.slice(-1)[0]; to make the separator clearly a string.
| const newOrder = toFormat.slice(0, -1); | ||
| const newSeparator = toFormat.slice(-1); | ||
| const oldOrder = fromFormat.slice(0, -1); | ||
| const oldSeparator = fromFormat.slice(-1); |
There was a problem hiding this comment.
Similar to line 12, fromFormat.slice(-1) produces an array, so oldSeparator becomes something like ['-']. Using this in split depends on array-to-string coercion; it would be clearer and safer to store the separator as a string by accessing the last element directly.
No description provided.