Skip to content

Solution#2693

Open
SashaGlazko wants to merge 2 commits into
mate-academy:masterfrom
SashaGlazko:develop
Open

Solution#2693
SashaGlazko wants to merge 2 commits into
mate-academy:masterfrom
SashaGlazko:develop

Conversation

@SashaGlazko

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are still functional issues that can break valid scenarios described in the task. The most critical blocker is in src/formatDate.js: in the second loop you iterate with for (let j = 0; j < arrDate.length; j++) while indexing toFormat[j], but toFormat includes the separator as an extra element and may not align with arrDate.length; this can lead to incorrect indexing and wrong output when formats differ. You should instead iterate over the date-part tokens in toFormat (e.g., up to toFormat.length - 1 or until toFormat[j] equals the separator), so that the output is always driven by the target format structure. A second issue is in normalizeYear: it currently returns a value only when targetYearFormat is 'YY' or 'YYYY', and otherwise falls through without returning anything, which can produce undefined if the function is reused or misused; it’s safer to add a default return (for example, returning the input unchanged or throwing an error for unsupported formats). These two problems are critical because they directly affect correctness and can cause the function to produce invalid dates in some valid input/output format combinations. Once you adjust the loop to be based on toFormat’s date tokens and ensure normalizeYear always returns a defined string, your solution will align well with the task requirements. You’ve already done a good job implementing separator changes, reordering, and year conversion—fixing these targeted issues will complete the functionality.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/formatDate.js Outdated
parts[fromFormat[i]] = arrDate[i];
}

for (let j = 0; j < arrDate.length; j++) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the second loop you iterate up to arrDate.length, but you’re indexing toFormat[j]. toFormat includes the separator as an extra element and may not align with arrDate.length, so you should iterate over the date tokens in toFormat (excluding the separator) instead of arrDate.length to avoid incorrect indexing or missing parts.

Comment thread src/formatDate.js
return resArr.join(toFormat.at(-1));
}

function normalizeYear(obj, targetYearFormat) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalizeYear has no default return value if targetYearFormat is neither 'YYYY' nor 'YY'. While current calls only use these two, it’s better to ensure this function always returns a string (or explicitly handle unexpected formats) to avoid accidental undefined results later.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved because it correctly implements all the required formatDate functionality and matches the specified behavior for the given examples. The function properly splits the input date using the separator defined by fromFormat, reorders the parts according to toFormat, and then rejoins them with the new separator. The year conversion logic is implemented accurately: YYYY → YY uses the last two digits, and YY → YYYY applies the 20YY rule for years under 30 and 19YY for others, which makes your handling of cases like 20/02/18 and 97/02/18 correct. Your normalizeYear helper consistently returns defined values for supported formats and throws on unsupported ones, preventing silent failures while respecting the assumption of correctly formatted inputs. There are no critical blockers or missing requirements, and the function behaves as expected across all described scenarios. Nice work bringing everything in line with the task requirements and previous feedback.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants