8000 Fix palindromes false positive test by Mclilzee · Pull Request #227 · TheOdinProject/javascript-exercises · GitHub
[go: up one dir, main page]
Skip to content

Fix palindromes false positive test #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 12, 2022
Merged

Fix palindromes false positive test #227

merged 1 commit into from
Feb 12, 2022

Conversation

@Mclilzee
Copy link
Member
@Mclilzee Mclilzee commented Feb 8, 2022 •
edited
Loading

As all palindromes have the exact same first and last letter, people only had to test for first and last letter equallity, and the only test where it should return false, it has the first and last letters being different. to pass the whole test people only had to test for first and last letter weither they are equal or not, which make mistakes like this pass the test :

const palindromes = function (input) {
  let
F440
 str = input.replace(/[^0-9a-z]/gi, "").toLowerCase();
  for (var i in str) {
    if (str[i] != str[str.length-1-i]) {
      return false;
    }
    return true;
  }
};

this mistake happens to a lot of people, where they return true inside rather than outside the loop. I myself did this mistake many times.

My change will make sure that the first and last letter of a none-palindrome sentence the same, which returns true if people only tested first and last letter. passing the tests now require people to do the palindrome test properly.

OoChip
OoChip approved these changes Feb 11, 2022
View reviewed changes
Copy link
@OoChip OoChip left a comment

Choose a reason for hiding this comment

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

look ok

09_palindromes/palindromes.spec.js
expect(palindromes('Animal loots foliated detail of stool lamina.')).toBe(true);
});
test.skip('doesn\'t just always return true', () => {
expect(palindromes('ZZZZ car, a man, a maraca.')).toBe(false);
Copy link
@OoChip OoChip Feb 11, 2022

Choose a reason for hiding this comment

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

look ok

xandora
xandora approved these changes Feb 12, 2022
View reviewed changes
Copy link
Member
@xandora xandora left a comment

Choose a reason for hiding this comment

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

Good catch, change looks good to me.

@xandora xandora merged commit 2f9f431 into TheOdinProject:main Feb 12, 2022
d2038 pushed a commit to d2038/javascript-exercises that referenced this pull request May 22, 2024
@xandora
Merge pull request TheOdinProject#227 from Mclilzee/main
e6c755b
Oussama5379 added a commit to Oussama5379/javascript-exercises that referenced this pull request Feb 1, 2025
@Oussama5379
Merge pull request TheOdinProject#227 from Mclilzee/main
dedaba9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xandora xandora xandora approved these changes

+1 more reviewer

@OoChip OoChip OoChip approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Mclilzee @xandora @OoChip
0