Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(totalIntegers): create exercise #443

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

NikitaRevenco
Copy link
Contributor

@NikitaRevenco NikitaRevenco commented Mar 23, 2024

Because

It was decided to add new recursion exercises

Previous

This PR

  • Adds exercise 15

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

@NikitaRevenco NikitaRevenco marked this pull request as draft March 23, 2024 14:56
@NikitaRevenco NikitaRevenco changed the title New exercise | Total Integers (15) New exercise | Total Integers Mar 23, 2024
@NikitaRevenco NikitaRevenco marked this pull request as ready for review March 24, 2024 14:32
Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

@NikitaRevenco
Copy link
Contributor Author

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

@MaoShizhong
Copy link
Contributor

Didn't know that method exists, how convenient!

Pssst...

@NikitaRevenco
Copy link
Contributor Author

Didn't know that method exists, how convenient!

Pssst...

Wait whaaaat xD

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't had time to assess the tests but at least in terms of how the solution is written, here are some suggestions.

15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
@NikitaRevenco
Copy link
Contributor Author

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.

@MaoShizhong
Copy link
Contributor

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.

Copy link
Contributor

@JoshDevHub JoshDevHub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some thoughts. Let me know if you have any questions.

15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
@NikitaRevenco NikitaRevenco changed the title (15) New exercise | Total Integers feat(totalIntegers): create exercise Aug 9, 2024
@MaoShizhong
Copy link
Contributor

@NikitaRevenco Any issues with the above comments from Josh?

@NikitaRevenco
Copy link
Contributor Author

@NikitaRevenco Any issues with the above comments from Josh?

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

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the comment below, I'm happy to approve once the tests and appropriate skips have been copied over to the other spec file 🔥

15_totalIntegers/solution/totalIntegers-solution.js Outdated Show resolved Hide resolved
@MaoShizhong MaoShizhong added the Status: Do Not Merge This PR should not be merged until further notice label Aug 20, 2024
Copy link
Contributor

@JoshDevHub JoshDevHub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than Mao's one live comment, I think everything looks good with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Do Not Merge This PR should not be merged until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants