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

Learn Functional Programming by Building a Spreadsheet - Step 74 Accepts Regex That Don't Meet Criteria #54793

Open
rbscharf93 opened this issue May 14, 2024 · 3 comments
Labels
new javascript course These are for issues dealing with the new JS curriculum scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. status: discussing Under discussion threads. Closed as stale after 60 days of inactivity. status: waiting triage This issue needs help from moderators and users to reproduce and confirm its validity and fix. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc.

Comments

@rbscharf93
Copy link

rbscharf93 commented May 14, 2024

Describe the Issue

Lesson asks you to, "declare a regex variable. Assign it a regular expression that matches a number (including decimal numbers) followed by a * or / operator followed by another number," but will accept regex' from users that don't satisfy this criteria (examples /\d/ and /5/, matching anything with just a number, and anything with a 5 in it).

Related issue #54327 to address test being too strict.

Opened from forum post: https://forum.freecodecamp.org/t/learn-functional-programming-by-building-a-spreadsheet-step-74/690100/2

Affected Page

https://www.freecodecamp.org/learn/javascript-algorithms-and-data-structures-v8/learn-functional-programming-by-building-a-spreadsheet/step-74

Your code

const highPrecedence = str => {
 const regex = /5/;
 return regex.test(str);
}

Expected behavior

Lesson should pass when Regex matches against a number, followed by a * or / operator, followed by another number, but matches any number. Regex's that pass but should not (likely not limited to):
/5/
/\d/
/\*/

Screenshots

No response

System

  • Device: [e.g. iPhone 6, Laptop]
  • OS: [e.g. iOS 14, Windows 10, Ubuntu 20.04]
  • Browser: [e.g. Chrome, Safari]
  • Version: [e.g. 22]

Additional context

No response

@rbscharf93 rbscharf93 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. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc. labels May 14, 2024
@jdwilkin4
Copy link
Contributor

It looks like the last test to could be updated to include a few more assertions like this

Your `highPrecedence` function should correctly check if the string matches the pattern of a number followed by a `*` or `/` operator followed by another number.

```js

assert.isTrue(highPrecedence("5*3"));
assert.isFalse(highPrecedence("5"));
assert.isTrue(highPrecedence("10/2"));
assert.isFalse(highPrecedence("*"));
```

But also, looking at this challenge again, I think it could be simplified. At this point, we should only be testing for the final result and allow campers to solve it however they want to.

Here is my proposed update

updated description

# --description--

In your `highPrecedence` function, you should use a regex to check if the provided string matches the pattern of a number followed by a `*` or `/` operator followed by another number. 

Your function should return a boolean value. Remember that you can use the `test()` method for this.

Each number, and the operator, should be in separate capture groups.

updated tests

Your `highPrecedence` function should return a boolean value.

```js
assert.isBoolean(highPrecedence("12*2"));
```

Your `highPrecedence` function should correctly check if the string matches the pattern of a number followed by a `*` or `/` operator followed by another number.

```js

assert.isTrue(highPrecedence("5*3"));
assert.isFalse(highPrecedence("5"));
assert.isTrue(highPrecedence("10/2"));
assert.isFalse(highPrecedence("*"));
```

I'll open this up for discussion so the other members can chime in with thoughts.

@jdwilkin4 jdwilkin4 added status: discussing Under discussion threads. Closed as stale after 60 days of inactivity. new javascript course These are for issues dealing with the new JS curriculum labels May 14, 2024
@AndrewYturaldi
Copy link
Contributor

I see this as the only test for the regular expression itself:

Your regex variable should be a regular expression.

assert.match(code, /const\s+highPrecedence\s*=\s*(\(\s*str\s*\)|str)\s*=>\s*{\s*const\s+regex\s*=\s*\//);

does this issue require the addition of more tests? and if so, should there be seperate tests for each regex that produces an unexpected behavior like /5/ and /\d/ or just one test that ensures the regex conforms to what was asked for (includes the two numbers, * symbol, / symbol, and 3 capture groups)?

@jdwilkin4
Copy link
Contributor

jdwilkin4 commented May 14, 2024

@AndrewYturaldi

The test you mentioned here only checks that the a regex variable was created and that some sort of regex is being used

assert.match(code, /const\s+highPrecedence\s*=\s*(\(\s*str\s*\)|str)\s*=>\s*{\s*const\s+regex\s*=\s*\//);

So basically it is testing this

const regex = /some-random-stuff-goes-here/

Currently this test is checking for the correct result

Please enter a properly functioning regex.

```js

assert.strictEqual(highPrecedence("5*3"), true);

```

but it right now allows for multiple incorrect answers

does this issue require the addition of more tests?

My proposal is to rewrite the lesson to allow for more correct answers and ensure that incorrect answers don't pass.
Right now, you have to create a regex variable and you have to return regex.test(/correct-regex-goes-here/)
But there are dozens of correct answers and this should be rewritten to allow for that.

So you can see my note here for my proposed changes
#54793 (comment)

just one test that ensures the regex conforms to what was asked for (includes the two numbers, * symbol, / symbol, and 3 capture groups)?

The thing we want to be carefully about is not using regex tests to check for this.
Because that is what is happening for the other lessons and it is difficult to maintain and only allows for a specific answer.

So that is why this is marked as open for discussion 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new javascript course These are for issues dealing with the new JS curriculum scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. status: discussing Under discussion threads. Closed as stale after 60 days of inactivity. status: waiting triage This issue needs help from moderators and users to reproduce and confirm its validity and fix. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc.
Projects
None yet
Development

No branches or pull requests

3 participants