javascript-exercises icon indicating copy to clipboard operation
javascript-exercises copied to clipboard

feat(totalIntegers): create exercise

Open nik-rev opened this issue 1 year ago • 7 comments

Because

It was decided to add new recursion exercises

Previous

This PR

  • Adds exercise 15

Issue

Related to #27265

Additional Information

Pull Request Requirements

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

nik-rev avatar Mar 23 '24 14:03 nik-rev

General but important comment to consider for this one - I'll let you have a think about how you want to approach this.

  1. No need to reinvent the wheel for isInteger
  2. 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

nik-rev avatar Mar 24 '24 17:03 nik-rev

Didn't know that method exists, how convenient!

Pssst...

mao-sz avatar Mar 24 '24 17:03 mao-sz

Didn't know that method exists, how convenient!

Pssst...

Wait whaaaat xD

nik-rev avatar Mar 24 '24 17:03 nik-rev

2 more general comments:

  • Don't forget to add .skip to 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 .solution.spec files don't have any .skipped tests in any of the previous exercises. But yeah that will have to be done when I end up adding all the tests to the regular spec files.

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.

nik-rev avatar Apr 07 '24 17:04 nik-rev

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 :+1:

Also fair point on the exercise generator tool - let's stick to trad. function expressions then.

mao-sz avatar Apr 07 '24 17:04 mao-sz

@nikitarevenco Any issues with the above comments from Josh?

mao-sz avatar Aug 18 '24 11:08 mao-sz

@nikitarevenco Any issues with the above comments from Josh?

Ok, went over all the comments just now and added the suggestions

nik-rev avatar Aug 18 '24 14:08 nik-rev