Skip to content

isogram#554

Open
gmarchese93 wants to merge 1 commit intomate-academy:mainfrom
gmarchese93:develop
Open

isogram#554
gmarchese93 wants to merge 1 commit intomate-academy:mainfrom
gmarchese93:develop

Conversation

@gmarchese93
Copy link

JEST requirements met

Copy link

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

Choose a reason for hiding this comment

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

Review Summary

Excellent work on the tests for the isIsogram function! I am approving your solution as it comprehensively covers all the requirements, including handling case-insensitivity and edge cases like the empty string.

As a minor suggestion for future improvement, the test description for 'Oops' in src/isIsogram.test.js mentions 'non-consecutive letters'. After being made lowercase, the repeating 'o's are actually consecutive. While the test itself is perfectly valid, using an input like 'aba' would align more precisely with that specific description.

This is just a small point on clarity, and your overall implementation is solid. Keep up the great work!


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

Footnotes

  1. Rate AI review example

expect(isIsogram('look')).toBe(false);
});

test('returns false when a word contains repeating non-consecutive letters', () => {

Choose a reason for hiding this comment

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

This is a great test case that correctly checks for case-insensitivity. However, the description ...repeating non-consecutive letters can be slightly confusing here. In 'Oops', the repeating letters ('o' and 'o' after making them lowercase) are actually consecutive. Your test case for 'Adam' is a perfect example of non-consecutive repeating letters.

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