Skip to content

Task solution#658

Open
OlhaLishenko wants to merge 3 commits into
mate-academy:mainfrom
OlhaLishenko:develop
Open

Task solution#658
OlhaLishenko wants to merge 3 commits into
mate-academy:mainfrom
OlhaLishenko:develop

Conversation

@OlhaLishenko

Copy link
Copy Markdown

No description provided.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because one explicit requirement from the task description is not yet covered by your test suite, which makes the solution incomplete. Your implementation of getCoinCombination is correct and your Jest tests for 0, 6, 17, and 50 cents are clear and well-structured, but the problem statement specifically calls out the example getCoinCombination(1) === [1, 0, 0, 0] as a required case. To fix this, add a Jest expectation in src/getCoinCombination.test.js (for example right after the 0-cents test) like:

expect(getCoinCombination(1)).toEqual([1, 0, 0, 0]);

This will ensure that all mandated examples from the description are enforced by tests and that your solution fully matches the spec. As a minor (non-blocking) improvement, you might also consider adding a couple more values (like 30 or 99 cents) for stronger coverage, but that is optional. Once you add the missing 1-cent test, your submission will fully satisfy the core requirements.


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

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

expect(getCoinCombination).toBeInstanceOf(Function);
});

it(`no coins should return [0, 0, 0, 0]`, () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The test suite is missing the required example from the task description: add a Jest test asserting getCoinCombination(1) === [1, 0, 0, 0]. For example, add a test similar to the others (e.g. after the 0 test): expect(getCoinCombination(1)).toEqual([1, 0, 0, 0]); This is required by the description/examples.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because one core requirement from the task description is currently not being tested correctly, which is a functional gap rather than a cosmetic issue. In src/getCoinCombination.test.js, the test titled something like one coin should return [1, 0, 0, 0] is calling getCoinCombination(0) instead of getCoinCombination(1), so the mandated example getCoinCombination(1) === [1, 0, 0, 0] is never actually asserted. This means the tests do not fully enforce the specified behavior, which is a blocker for accepting the solution.

To fix this:

  • Update that test so the call is getCoinCombination(1) and the expected result remains [1, 0, 0, 0].
  • Optionally, update the test description to say one cent should return [1, 0, 0, 0] so the input is explicit and less ambiguous.

Your implementation file is correctly left unchanged and already satisfies the logic requirements; once the test is corrected to cover the 1-cent case, the solution will meet the task standards.


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

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/getCoinCombination.test.js Outdated
});

it(`one coin should return [1, 0, 0, 0]`, () => {
const result = getCoinCombination(0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test is intended to assert the required example getCoinCombination(1) === [1, 0, 0, 0], but it calls getCoinCombination(0). Change the call to getCoinCombination(1) so the mandated case from the task description is actually tested.

expect(result).toEqual([0, 0, 0, 0]);
});

it(`one coin should return [1, 0, 0, 0]`, () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Optional clarity suggestion: the description one coin should return [1, 0, 0, 0] could be changed to one cent should return [1, 0, 0, 0] to make the input explicit after you update the test value.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am approving your solution because it correctly computes the minimal coin combination for US coins and fully matches the required examples, including 0, 1, 6, 17, and 50 cents. Your implementation is clear, uses a straightforward greedy approach, and the tests accurately verify both the output values and the correct order [pennies, nickels, dimes, quarters].

This approval includes a small educational override note: there are only minor improvement opportunities, not functional errors. One minor point is wording in your test description for the 1-cent case in src/getCoinCombination.test.js—changing the phrasing from “one coin” to “one cent” would make the intent clearer to readers. You could also consider adding a couple of extra tests for larger values (for example, 37 or 99) to further build confidence that the greedy approach always yields the minimal combination. Overall, you’ve met the core requirements well, and your solution is approved—nice work building both a correct function and solid tests around it.


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

Footnotes

  1. Rate AI review example

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