(13) New exercise | Factorial

[nikitarevenco]

Because

It was decided to add new recursion exercises

This PR

• Adds exercise 13

Issue

Related to #27265

Additional Information

Pull Request Requirements

• I have thoroughly read and understand The Odin Project Contributing Guide
• The title of this PR follows the location of change: brief description of change format, e.g. 01_helloWorld: Update test cases
• The Because section summarizes the reason for this PR
• The This PR section has a bullet point list describing the changes in this PR
• If this PR addresses an open issue, it is linked in the Issue section
• If this PR includes any changes that affect the solution of an exercise, I've also updated the solution in the /solutions folder

[MaoShizhong]

EDIT: The more I think about it, the more I feel that accepting strings is unnecessary for the intent of this exercise. The point of the exercise is to practise recursion, and it's the first exercise for that to boot.

Therefore, learners should be focusing on wrestling with the recursion part, not type checking and various additional unrelated things to prevent edge cases like factorial([4]) being valid without an Array.isArray rejection. Accepting only numbers allows for only !Number.isInteger(n) and n < 0 rejections. Far more relevant and intuitive, and does not reduce complexity for the actual point of the task, which is recursion.

---

Additional scenarios that need to be tested:

• Accept integers with .0 e.g. 5.0 or '5.0'. String would be better as we'd only need a single test case for that (needing to convert it to a number would mean it also effectively tests 5.0)
• Reject string floats e.g. '8.9'
• Reject some other invalid value e.g. 'foo'

Though having said that, I'm wondering if there's much actual value in accepting string values at all. Since the exercise is primarily about recursion, I'd probably argue that it makes more sense to only accept numbers (so '4' or '4.0' would be rejected). That would also solve edge cases like factorial([]) and factorial([7]) being the equivalents of factorial(0) and factorial(7) respectively, as arrays coerce to strings which then cleanly convert to legit numbers in those two cases.

[nikitarevenco]

EDIT: The more I think about it, the more I feel that accepting strings is unnecessary for the intent of this exercise. The point of the exercise is to practise recursion, and it's the first exercise for that to boot.

Therefore, learners should be focusing on wrestling with the recursion part, not type checking and various additional unrelated things to prevent edge cases like factorial([4]) being valid without an Array.isArray rejection. Accepting only numbers allows for only !Number.isInteger(n) and n < 0 rejections. Far more relevant and intuitive, and does not reduce complexity for the actual point of the task, which is recursion.

Additional scenarios that need to be tested:

• Accept integers with .0 e.g. 5.0 or '5.0'. String would be better as we'd only need a single test case for that (needing to convert it to a number would mean it also effectively tests 5.0)
• Reject string floats e.g. '8.9'
• Reject some other invalid value e.g. 'foo'

Though having said that, I'm wondering if there's much actual value in accepting string values at all. Since the exercise is primarily about recursion, I'd probably argue that it makes more sense to only accept numbers (so '4' or '4.0' would be rejected). That would also solve edge cases like factorial([]) and factorial([7]) being the equivalents of factorial(0) and factorial(7) respectively, as arrays coerce to strings which then cleanly convert to legit numbers in those two cases.

I agree that it would be better to make the learner not have to deal with strings and instead focus on the recursion aspect. Should this approach be employed for all future exercises?

[nikitarevenco]

EDIT: The more I think about it, the more I feel that accepting strings is unnecessary for the intent of this exercise. The point of the exercise is to practise recursion, and it's the first exercise for that to boot.

Therefore, learners should be focusing on wrestling with the recursion part, not type checking and various additional unrelated things to prevent edge cases like factorial([4]) being valid without an Array.isArray rejection. Accepting only numbers allows for only !Number.isInteger(n) and n < 0 rejections. Far more relevant and intuitive, and does not reduce complexity for the actual point of the task, which is recursion.

Additional scenarios that need to be tested:

• Accept integers with .0 e.g. 5.0 or '5.0'. String would be better as we'd only need a single test case for that (needing to convert it to a number would mean it also effectively tests 5.0)
• Reject string floats e.g. '8.9'
• Reject some other invalid value e.g. 'foo'

Though having said that, I'm wondering if there's much actual value in accepting string values at all. Since the exercise is primarily about recursion, I'd probably argue that it makes more sense to only accept numbers (so '4' or '4.0' would be rejected). That would also solve edge cases like factorial([]) and factorial([7]) being the equivalents of factorial(0) and factorial(7) respectively, as arrays coerce to strings which then cleanly convert to legit numbers in those two cases.

Also, instead of making it return undefined lets have the learner make it throw an error! The curriculum does not officially cover how to throw, but they can look it up. They should know this is something that can be done.

[MaoShizhong]

@nikitarevenco I think that an exercise should focus on challenging a learner on things related to its intent.

For example, this exercise's main intents are:

• Factorial
• Recursive

The recursion part is obvious. The factorial part requires we only handle valid values (non-negative integers). Therefore, the exercise should be made to challenge on those parts. Rejecting non-negative integers is a key part of the factorial process, therefore is relevant to be included.

Strings can be converted to numbers before being passed in as arguments - it does not need to be handled by the function itself. Adding that capability in is an extra that's unrelated to the exercise's purpose, as it doesn't challenge the learner on any of the above key concepts (factorial/recursion). Those are what we want to challenge the learner on with this exercise, not type conversion.

Regarding throwing an error instead of returning undefined, I also think the above is relevant. We should keep to challenging on the intent of the exercise.

[MaoShizhong]

For future reference as well, if you want to increase the complexity of an exercise, you do so by making the relevant problem itself more challenging.

If you try to do so by adding in arbitrary unrelated extras, that doesn't make the exercise more challenging in a beneficial way given what its purpose is. That's just arbitrarily putting in extra stuff for no reason other than "to make it more complex".

[nikitarevenco]

For future reference as well, if you want to increase the complexity of an exercise, you do so by making the relevant problem itself more challenging.

If you try to do so by adding in arbitrary unrelated extras, that doesn't make the exercise more challenging in a beneficial way given what its purpose is. That's just arbitrarily putting in extra stuff for no reason other than "to make it more complex".

Cool! I updated it.

[nikitarevenco]

For future reference as well, if you want to increase the complexity of an exercise, you do so by making the relevant problem itself more challenging.

If you try to do so by adding in arbitrary unrelated extras, that doesn't make the exercise more challenging in a beneficial way given what its purpose is. That's just arbitrarily putting in extra stuff for no reason other than "to make it more complex".

Shall I start working on these now -

• contains() from the JS challenges
• totalIntegers() from the JS challenges
• sumSquares() from the JS challenges
• #flatten from the Ruby challenges

[MaoShizhong]

For future reference as well, if you want to increase the complexity of an exercise, you do so by making the relevant problem itself more challenging.
If you try to do so by adding in arbitrary unrelated extras, that doesn't make the exercise more challenging in a beneficial way given what its purpose is. That's just arbitrarily putting in extra stuff for no reason other than "to make it more complex".

Shall I start working on these now -

• contains() from the JS challenges
• totalIntegers() from the JS challenges
• sumSquares() from the JS challenges
• #flatten from the Ruby challenges

I would say start working on the next intended JS exercise (so we do them one by one), and include a link to this PR in that one's text.

I think it would be most sensible to only add these exercises to the repo once we've agreed on the state of all proposed exercises, then we can merge them en masse. I'll probably discuss with the team what the best plan of action would be, but for the time being, this feels more sensible than merging an exercise that won't be used by the curriculum until all exercises are merged anyway.

It'll also give other maintainers a chance to look at and add their reviews if necessary.

[JoshDevHub]

More general comments rather than things I think for sure should be included. Curious to know what both of you think @nikitarevenco @MaoShizhong

I also agree with Mao on not having the functions necessarily be flexible with strings and stuff. I realize the other exercises in this repo often do things like that, but this is just to give people practice with typecasting data. It's not how functions tend to be written in the "real world", and it's also not particularly relevant for the task at hand with this group of exercises (wrapping your mind around recursion).

[nikitarevenco]

@MaoShizhong @JoshDevHub

I think once all of these new exercises get merged it wouldn't make sense to keep them separate from the project so I propose to also convert the tasks in the project into TDD exercises too and remove the project entirely