foundations/object_basics/03_fibonacci/solution/fibonacci-solution.js: Fix solution error handling for fibonacci() when count is NaN - #677
Conversation
Previous logic allows fibonacci('hello') to return 1 instead of 'OOPS'
mao-sz
left a comment
There was a problem hiding this comment.
Thanks for suggesting this. I'm not actually convinced by it though. The point of the exercise isn't to make a robust function for real world usage that covers most/all possible cases, but rather to just practise some fundamentals in order to pass all the tests, for which NaN handling isn't.
That being said, looking at the current tests, I'm not a fan of the last few that deal with strings. Not really seeing the point having them in this exercise specifically; there are plenty of opportunities for parsing/converting strings to numbers throughout the whole curriculum.
I'd like to propose a different and slightly larger change to this exercise. If you're happy to do it, then just do the work in this branch. Otherwise, you can close the PR and I'll get the work handled another way.
I think instead of testing for string->number conversion, we should amend the instructions and tests so that if the argument is a negative number, NaN or not a number-type at all, return "OOPS", i.e. only proceed with positive numbers.
Means it focuses more on doing the fibonacci stuff and just shoving aside all other types of values, no other type-specific behaviour.
Because
Previous logic for solution allows fibonacci('hello') to return 1 instead of 'OOPS'. While this is not a breaking change, I felt as though it was relevant when looking at the conditional logic.
This PR
!countto if conditional block that returns"OOPS"so it now readsif (!count || count < 0) return "OOPS"if (count == 0) return 0beforeif (!count || count < 0) return "OOPS"to allow proper handling ofcount = 0Issue
Closes #XXXXX
Additional Information
Pull Request Requirements
location of change: brief description of changeformat, e.g.01_helloWorld: Update test casesBecausesection summarizes the reason for this PRThis PRsection has a bullet point list describing the changes in this PRIssuesection/solutionsfolder