Skip to content
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.

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

Regular Expression Password Verification #37932

Closed
jondparadise opened this issue Dec 14, 2019 · 13 comments · Fixed by #38023
Closed

Regular Expression Password Verification #37932

jondparadise opened this issue Dec 14, 2019 · 13 comments · Fixed by #38023
Labels
scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory.

Comments

@jondparadise
Copy link

https://www.freecodecamp.org/learn/javascript-algorithms-and-data-structures/regular-expressions/positive-and-negative-lookahead

I am growing increasingly frustrated with the section on regular expressions. They are a notoriously difficult concept to grasp and special attention should be paid to ensure that your regex requirements, test cases and solutions are all compatible with one another.

Regex Requirements:
Use lookaheads in the pwRegex to match passwords that are greater than 5 characters long, do not begin with numbers, and have two consecutive digits.

An Accepted Solution:

let sampleWord = "astronaut";
let pwRegex = /^(?=\w{6})(?=\D+\d{2})/; // Change this line
let result = pwRegex.test(sampleWord);

The issue:
let sampleWord = "astr1on11aut";
This sample password is longer than 5 characters, does not begin with a number and contains two consecutive digits. However, it returns FALSE when using the accepted solution.

@jondparadise jondparadise added scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. status: waiting triage This issue needs help from moderators and users to reproduce and confirm its validity and fix. labels Dec 14, 2019
@ojeytonwilliams
Copy link
Contributor

This is quite a tricky problem. I've come up with a couple of regexes that pass all the tests (and accept astr1on11aut):

let pwRegex1 = /^\D(?=\w{5})(?=\w*\d{2})/;
let pwRegex2 = /^(?=\w{6})(?=\D\w*\d{2})/;

Are those reasonable things to expect a beginners to come up with and are they actually correct? If so, I think we should add astr1on11aut to the tests and update the solution to one of those (I think pwRegex2 is the easiest to understand).

@moT01
Copy link
Member

moT01 commented Dec 15, 2019

What about just adding something to the instructions that says "all numbers have to be consecutive" or something - that would make this an easy fix, and not complicate the challenge.

@jondparadise
Copy link
Author

I concur with Tom on this. The most glaring issue for me was the way the question was presented. It wasn't descriptive enough to prevent me from thinking of this alternative test case.

@ojeytonwilliams
Copy link
Contributor

What about just adding something to the instructions that says "all numbers have to be consecutive"

I think that requires a more complicated regex. What did you have in mind?

@SavvyShah
Copy link
Contributor

SavvyShah commented Dec 18, 2019

I think I could help you out. Correct me if I am wrong. Consider the code below. What do you think would it return?

let quit = "qu";
let noquit = "qt";
let quRegex= /(?=q)(?=qu)/; //true

console.log(quRegex.test(quit));

Also, try changing the code to this and see the return value. Did you get the problem?

let quRegex= /(?=q)(?=q)/; //true

And also to

let quRegex= /(?=q)(?=u)/; //false

The first(?=q) and second(?=u) lookahead starts from the beginning and they must match the whole pattern exactly. It is not like regular expressions that they would find a match from anywhere whether it is middle of the word.

Freecodecamp as Tom says wants number to appear only in pair whenever they appear. But you seem to think that it should just match 2 consecutive digits so for that change your code to below and that would fix your problem.

let pwRegex = /^(?=\w{6})(?=\w+\d{2})/; // Change this line

@ojeytonwilliams
Copy link
Contributor

ojeytonwilliams commented Dec 18, 2019

let pwRegex = /^(?=\w{6})(?=\w+\d{2})/; 

That regex is a bit confusing, isn't it?

It matches 123abc and a123bc, but not 12abcd. So, in words

Must have two consecutive digits, but the first character is not counted as a digit.

As I said, I think just asking for two consecutive digits requires a regex at least as complicated as the original instructions.

@SavvyShah
Copy link
Contributor

SavvyShah commented Dec 19, 2019

But you can have a look at your own solution and if I insert \D then it would work. Also notice that both of us have used w instead of D in our solution which makes this work for astr1on11aut.

let pwRegex1 = /^\D(?=\w{5})(?=\w*\d{2})/; (Oliver)
let pwRegex = /^(?=\D\w{5})(?=\w+\d{2})/; (Shubham)
let pwRegex = /^(?=\w{6})(?=\D+\d{2})/; // Change this line(Original poster)

Now it would return false for 123abc and 12abcd. But I do agree that it gets a little complex.

@ojeytonwilliams
Copy link
Contributor

I'm happy to go with updating the solution (with either Shubham's or mine, I think they're equivalent) and adding astr1on11aut to the tests. We would not need to change the instructions in this case.

It will be a hard challenge, but perhaps not too hard. @moT01 @jondparadise what do you think?

@jondparadise
Copy link
Author

I happy to have any solution. I'm just looking for more detailed instructions that will assist others in creating a pattern that satisfies the question's requirements. Thanks for taking the time to address this!

@ojeytonwilliams
Copy link
Contributor

@ShubhamCanMakeCommit do you think you'll have the time to create a PR with your solution and the extra test?

@SavvyShah
Copy link
Contributor

I think we should ask @moT01 . If you both are ready to go with this I have made changes you can see this I have made following changes and I am ready to make a pull request.

  • Removed some redundant tests that would pass if the others would pass and have kept enough tests for understanding.
  • Added "astr1on11aut" to the test
  • Updated the solution.

Let me know what you guys think. @jondparadise @ojeytonwilliams @moT01

Check the changes below.
My freeCodeCamp repo

@ojeytonwilliams
Copy link
Contributor

Looks good! I recommend creating a new (non-master) branch, though. It's easier to keep everything in the right state, if you do.

@moT01
Copy link
Member

moT01 commented Jan 7, 2020

Sorry for the late response, I think that will probably work @ShubhamCanMakeCommit. Part of me wants to say that that's too complicated, but it was already equally difficult before.

Make the PR when your ready and we can give it a final look 👍

@ilenia-magoni ilenia-magoni removed the status: waiting triage This issue needs help from moderators and users to reproduce and confirm its validity and fix. label Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants