Freecodecamp: Quality Assurance Projects - Metric-Imperial Converter Example Project accepts invalid input

[fuzzyray]

Describe your problem and how to reproduce it:
I am working through the Unit and Functional tests for the Metric Imperial Converter and using the example project to verify my understanding. The example project accepts invalid inputs that are supposed to be rejected according to what is being filled out in the test suite. The specific one being tested is double fraction input. From the 2_functional-test.js file:

      test('Convert 3/7.2/4kg (invalid number)', function(done) {

        //done();
      });

While it is not listed as a unit or functional test the example will also accept division by zero i.e. 3/0L

Add a Link to the page with the problem:
https://metric-imperial-converter--freecodecamp.repl.co/

Tell us about your browser and operating system:

• Browser Name: Mozilla Firefox
• Browser Version: 80.0
• Operating System: Windows 10

If possible, add a screenshot here (you can drag and drop, png, jpg, gif, etc. in this box):

[moT01]

So I think that the example project just needs to be changed to correctly handle these inputs. Negative numbers also work I noticed.

[RandellDawson]

@scissorsneedfoodtoo Since you are working on adding this project to a separate repo, could you also update the example project to correctly reject these invalid inputs?

[scissorsneedfoodtoo]

Thanks for bringing this up @fuzzyray.

This is something we've discussed in an older issue but never really came to a consensus.

Could you all take a look at this when you get a chance?: https://resonant-stupendous-wall.glitch.me/

For inputs like 13/13/13lbs it throws invalid number, and for 1a3lbs it throws invalid number and input.

It also accepts negative inputs, so long as the equation results in a whole number. So while -5mi will throw invalid number, -5*-5mi returns the following:

{ 
  "initNum": 25, 
  "initUnit": "mi", 
  "returnNum": 40.2335, 
  "returnUnit": "km", 
  "string": "25 miles converts to 40.2335 kilometers" 
}

If that sounds good I'll go ahead and update the solution.

[fuzzyray]

That sounds good, looking back through the unit testing and requirements. the only explicit math expression that it looks to want to declare as invalid is the double fraction. So looking at it and feeding it various things, it looks like it is working to correctly to me. The only additional expression that I personally would consider invalid is the division by 0 which on the project will return "Infinity miles converts to Infinity kilometers"

[scissorsneedfoodtoo]

Great, thank you for going through that and testing it out @fuzzyray.

Also, great catch with the dividing by 0 problem. That probably doesn't have to be a strict requirement with user stories and tests, but it would be good to handle those cases in the example project.

I'll go ahead and fix that in my fork of the project. Once that's approved by another team member or two, we'll make sure those changes are reflected in the live example project.

Update: Turned out to be a quick fix. Now things like 3/0mi return invalid number: https://resonant-stupendous-wall.glitch.me/

Could you take a look at this @RandellDawson and @moT01?

[SaintPeter]

I discovered another issue. Invalid input like a "double fraction":
https://metric-imperial-converter--freecodecamp.repl.co/api/convert?input=1//2gal

Causes a server error:

SyntaxError: Value expected (char 3)
    at createSyntaxError (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1705:17)
    at parseEnd (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1669:13)
    at parseParentheses (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1655:12)
    at parseNumber (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1626:12)
    at parseObject (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1607:12)
    at parseMatrix (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1532:12)
    at parseSingleQuotesString (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1433:12)
    at parseDoubleQuotesString (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1382:12)
    at parseSymbol (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1270:12)
    at parseCustomNodes (/home/runner/Metric-Imperial-Converter/node_modules/mathjs/lib/expression/parse.js:1239:12)

[scissorsneedfoodtoo]

Thanks for catching this @SaintPeter. This was fixed in the updates I made to https://resonant-stupendous-wall.glitch.me/.

Could you try some other invalid inputs with that project and let me know if you run into anything else?

[SaintPeter]

Here is a fun one:
https://resonant-stupendous-wall.glitch.me/api/convert?input=9,1.2/0mi

SyntaxError: Unexpected operator , (char 2)
    at createError (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/mathjs/6.2.3/node_modules/mathjs/lib/expression/parse.js:1722:17)
    at parseStart (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/mathjs/6.2.3/node_modules/mathjs/lib/expression/parse.js:553:15)
    at string (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/mathjs/6.2.3/node_modules/mathjs/lib/expression/parse.js:83:14)
    at parse (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/typed-function/1.1.1/node_modules/typed-function/typed-function.js:1085:85)
    at string (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/mathjs/6.2.3/node_modules/mathjs/lib/expression/function/evaluate.js:55:14)
    at Object.evaluate (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/typed-function/1.1.1/node_modules/typed-function/typed-function.js:1085:85)
    at ConvertHandler.getNum (/app/controllers/convertHandler.js:35:27)
    at /app/routes/api.js:21:43
    at Layer.handle [as handle_request] (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/express/4.16.4/node_modules/express/lib/router/layer.js:95:5)
    at next (/rbd/pnpm-volume/7a9f5196-20fc-430b-bde7-d1b287f71bc4/node_modules/.registry.npmjs.org/express/4.16.4/node_modules/express/lib/router/route.js:137:13)

That appears to break JS's parser. My thought is to wrap the whole parse block in a try/catch block and return an invalid number on any trip of it. We're just never going to "think" our way out of bad input. Trying to catch all the special cases is madness.

[SaintPeter]

I took a stab at this again. This is very much an "onion" problem - many layers and they all make you cry.

Here are the assumptions:
1) Units are at lest 1 alphabetic characters preceded by the start of the input or a non-alphabet character and ending at the end of the input
2) The numbers are anything left in the string once we remove the units with the prior characteristics
3) A number is made up of numeric digits and zero or one decimals
4) Division is always a / preceded by and succeeded by a single digit.

// Takes a string in, gives a true/false out
  this.numberChecker= function (num){
    // Check for non-digits and non-periods
    if(num.match(/[^0-9.]/gi)) {
      return false;
    }

    // Check for doubled periods
    let result = num.match(/\./gi);
    if(result && result.length > 1) {
      return false;
    }

    return true;
  }

  let re_units = /(?<=^|[^a-z])([a-z]+)$/ig
  this.getNum = function(input) {
    // get and remove units
    let noUnits = input.replace(re_units,'')

    // No number passed
    if(noUnits.length === 0) {
      return 1;
    }

    // Check for division
    // Valid division is a slash with a number
    // before and after
    let parts = noUnits.split(/(?<=\d)\/(?=\d)/ig);
    if(parts.length === 2) {
      if(!this.numberChecker(parts[0]) || !this.numberChecker(parts[1])) {
        return null;
      }
      // Division Present
      try {
        let numerator = parseFloat(parts[0]);
        let denominator = parseFloat(parts[1]);

        // Catch divide by zero
        if(denominator === 0) {
          return null;
        }
        return numerator/denominator;
      } catch(e) {
        return null;
      }
    } else {
      if(this.numberChecker(noUnits)) {
        try {
          return parseFloat(noUnits)
        } catch(e) {
          return null;
        }
      }
      return null;
    }
  };

[scissorsneedfoodtoo]

@SaintPeter, awesome, thank you for looking into all that and coming up with some clever code to validate the input before doing any conversions.

I'll see if I can work what you have into the https://resonant-stupendous-wall.glitch.me/ example.

[nhcarrigan]

With the migration to the demo-projects repo, this can be closed by https://github.com/freeCodeCamp/demo-projects/pull/37/ right?

[SaintPeter]

Yes, the improved demo project should deal with these issues better.