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

solution #389

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

solution #389

wants to merge 2 commits into from

Conversation

SrTrace
Copy link

@SrTrace SrTrace commented Oct 18, 2024

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

It looks like there are some critical issues with the test cases that need to be addressed to ensure the function behaves correctly. Please review the logic for handling US coin denominations and update the test descriptions and expected results accordingly. Keep up the good work, and once these issues are resolved, your code will be even stronger! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

function getCoinCombination(cents) {
if (cents < 0) {
throw new Error('Not valid argument');

Choose a reason for hiding this comment

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

The error message 'Not valid argument' could be more descriptive. Consider specifying that the argument must be a non-negative number.

@@ -7,4 +7,37 @@ describe('getCoinCombination', () => {
expect(getCoinCombination)
.toBeInstanceOf(Function);
});

it('should be non-negative input integer', () => {

Choose a reason for hiding this comment

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

The test description 'should be non-negative input integer' could be more precise. Consider rephrasing it to something like 'should throw an error for negative input'.

Comment on lines 24 to 25
it(`should return '[1, 0, 0, 0]' for 1 cents`, () => {
expect(getCoinCombination(1)).toEqual([1, 0, 0, 0]);

Choose a reason for hiding this comment

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

The test description 'should return '[1, 0, 0, 0]' for 1 cents' has a minor grammatical issue. It should be '1 cent' instead of '1 cents'.

Comment on lines 28 to 29
it(`should return '[1, 1, 0, 0]' for 6 cents`, () => {
expect(getCoinCombination(6)).toEqual([1, 1, 0, 0]);

Choose a reason for hiding this comment

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

The test description 'should return '[1, 1, 0, 0]' for 6 cents' is incorrect based on typical US coin denominations. The expected result should likely be '[1, 0, 1, 0]' for 6 cents, assuming the denominations are pennies, nickels, dimes, and quarters.

Copy link
Author

Choose a reason for hiding this comment

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

i don't understand this issue. Re-phrase it on more time

Comment on lines 32 to 33
it(`should return '[1, 1, 1, 0]' for 16 cents`, () => {
expect(getCoinCombination(16)).toEqual([1, 1, 1, 0]);

Choose a reason for hiding this comment

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

The test description 'should return '[1, 1, 1, 0]' for 16 cents' is incorrect based on typical US coin denominations. The expected result should likely be '[1, 1, 0, 1]' for 16 cents, assuming the denominations are pennies, nickels, dimes, and quarters.

Copy link
Author

Choose a reason for hiding this comment

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

?? 1 dime = 10 cents > 16 - 10 = 6
?? 5 nikel = 5 cents > 6 - 5 = 1
?? 1 cent = 1 cent
result [1cent, 1nikel, 1dime, 0quarter]

Comment on lines 36 to 37
it('should return [1, 1, 1, 1] for 41 cents', () => {
expect(getCoinCombination(41)).toEqual([1, 1, 1, 1]);

Choose a reason for hiding this comment

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

The test description 'should return [1, 1, 1, 1] for 41 cents' is incorrect based on typical US coin denominations. The expected result should likely be '[1, 1, 1, 1]' for 41 cents, assuming the denominations are pennies, nickels, dimes, and quarters. However, this test case seems to be correct, so please verify the logic of the function.

Comment on lines 40 to 41
it(`should return '[0, 0, 0, 3]' for 75 cents`, () => {
expect(getCoinCombination(75)).toEqual([0, 0, 0, 3]);

Choose a reason for hiding this comment

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

The test description 'should return '[0, 0, 0, 3]' for 75 cents' is incorrect based on typical US coin denominations. The expected result should likely be '[0, 0, 0, 3]' for 75 cents, assuming the denominations are pennies, nickels, dimes, and quarters. However, this test case seems to be correct, so please verify the logic of the function.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

The function getCoinCombination is well-implemented, correctly calculating the coin combinations needed for a given amount of cents. The error handling for negative input is appropriate, and the test cases are comprehensive, covering various scenarios. While the code is approved to help you progress in your learning journey, please review the comments to refine your code further. Address any critical issues in future projects to enhance your skills. Keep up the good work and continue improving! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants