(15) New exercise | Total Integers #443
Conversation
nikitarevenco
changed the title
There was a problem hiding this comment.
General but important comment to consider for this one - I'll let you have a think about how you want to approach this.
- No need to reinvent the wheel for isInteger
- You only test nested arrays but what if there are objects within the array(s) that contain integer values?
e.g.
[4, 6, { a: 1, b: { a: 10, b: 11 } }, 9]
Didn't know that method exists, how convenient! Good point about the objects with numbers, that certainly makes the challenge more interesting |
|
Wait whaaaat xD |
Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>
Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>
Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>
There was a problem hiding this comment.
2 more general comments:
- Don't forget to add
.skipto all but the 1st test in the suite. - I know the earlier exercises use a traditional function expression but I think it may be sensible to have these new exercises use arrow functions, and also for the non-solution template. The older exercises can always be updated another time. @JoshDevHub, thoughts on having the exercises use arrow functions instead of traditional func expressions?
For the first point, I am pretty sure that the Also I agree that it would be nice to have them all be arrow functions however I think consistency is important and it would be better to update them in a different PR. Also considering that the exercise generator tool would need to be updated too it would certainly have to all be in a separate PR. |
|
Ah yes, the solution test suite doesn't. As long as the skips get added when you copy over for the non-solution test suite, then all is good 👍 Also fair point on the exercise generator tool - let's stick to trad. function expressions then. |
| @@ -0,0 +1,21 @@ | |||
| const totalIntegers = function (obj, count = 0) { | |||
There was a problem hiding this comment.
With count not actually being passed to any recursive calls, I think it's better to just define it in the function body with a let count = 0 somewhere.
| @@ -0,0 +1,21 @@ | |||
| const totalIntegers = function (obj, count = 0) { | |||
| const isObject = (value) => typeof value === 'object' && value !== null; | |||
There was a problem hiding this comment.
This helper definition should be moved outside of the recursive function's body. This is because every time this function is run, there must be a new isObject() function object created for the current scope. This is wasteful -- each recursive call could just be using the same isObject() helper function if it were defined outside this function body.
This isn't super meaningful in the context of this specific problem (ie none of the examples are going to have performance issues because of this), but it is good practice to avoid defining constant factors (like this helper function) inside of loops or recursive functions.
| const totalIntegers = function (obj, count = 0) { | ||
| const isObject = (value) => typeof value === 'object' && value !== null; | ||
|
|
||
| if (typeof obj !== 'object' || obj === null) { |
There was a problem hiding this comment.
Seems to me this is just checking that the object is not an object. Using your helper for this feels cleaner
| if (typeof obj !== 'object' || obj === null) { | |
| if (!isObject(obj)) { |
Because
It was decided to add new recursion exercises
Previous
This PR
Issue
Related to #27265
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