Skip to content

Rozwiązanie zadania Split Integer#1028

Open
JoAnna578 wants to merge 4 commits into
mate-academy:mainfrom
JoAnna578:develop
Open

Rozwiązanie zadania Split Integer#1028
JoAnna578 wants to merge 4 commits into
mate-academy:mainfrom
JoAnna578:develop

Conversation

@JoAnna578
Copy link
Copy Markdown

Implementacja funkcji splitInteger w src/splitInteger.js.
Funkcja dzieli liczbę całkowitą na określoną liczbę części.

Copy link
Copy Markdown

@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.

Review Summary

I am requesting changes because the current tests do not fully validate the required behavior and include an invalid input case. In src/splitInteger.test.js, one test title says the value is divisible but calls splitInteger(17, 4) (line where it’s used) — 17 is not divisible by 4, and it only checks array length; you must assert the exact distribution [4, 4, 4, 5] and also verify sum, integer elements, sortedness, and max - min <= 1. Another test calls splitInteger(17, 0) (zero parts), which violates the “positive integers” input assumption; replace it with a valid “value < numberOfParts” case like splitInteger(2, 5) and assert [0, 0, 0, 1, 1] plus the required properties. For the existing splitInteger(32, 6) example, keep toEqual([5, 5, 5, 5, 6, 6]) but also add checks for length, sum equals 32, integer elements, and max - min <= 1. To meet the spec, also add exact-output tests for divisible cases such as splitInteger(8, 1) => [8] and splitInteger(6, 2) => [3, 3] and ensure each test verifies length, sum, all integers, non-decreasing order, and the max - min <= 1 constraint. Once these assertions are added and the invalid input test is removed/replaced, the suite will properly cover the checklist; nice job using Jest and including a correct example for (32,6) so far—finish tightening the tests and you’ll be set.


✨ 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/splitInteger.test.js Outdated
Comment on lines +7 to +8
expect(splitInteger(17, 4))
.toHaveLength(4);
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 calls splitInteger(17, 4) but the test title says "if a value is divisible by a numberOfParts" — 17 is not divisible by 4. Also this test only asserts the returned array length. Per the task requirements you must assert the actual distribution (for (17,4) the expected array is [4,4,4,5]) or use a truly divisible example such as splitInteger(8, 1) or splitInteger(6, 2). Additionally, each test should check the other required properties: sum equals the input value, all elements are integers, array is sorted non-decreasingly, and max - min <= 1.

Consider changing this assertion to one of the following approaches:

  • If you want to keep the example (17, 4):
const parts = splitInteger(17, 4);
expect(parts).toEqual([4, 4, 4, 5]);
expect(parts).toHaveLength(4);
expect(parts.reduce((a, b) => a + b, 0)).toBe(17);
expect(parts.every(Number.isInteger)).toBe(true);
expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);
  • Or change the input to a divisible case like splitInteger(6, 2) and assert toEqual([3, 3]) plus the other properties.

This is necessary to satisfy these checklist items: exact example outputs, sum preservation, integer elements, sortedness, and max-min difference requirement.

Comment thread src/splitInteger.test.js Outdated
Comment on lines +12 to +13
expect(splitInteger(32, 6))
.toEqual([5, 5, 5, 5, 6, 6]);
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 assertion toEqual([5, 5, 5, 5, 6, 6]) correctly matches the provided example for splitInteger(32, 6) and should be kept. However, the task requires tests to also verify the general properties for returned arrays (length correctness, integer elements, sorted non-decreasing order, max - min <= 1, and sum equals the original value). Relying on a single toEqual check covers content but you should add explicit assertions for those properties (or add separate tests that assert them) so the implementation is fully validated against the checklist.

Example additional assertions:

const parts = splitInteger(32, 6);
expect(parts).toHaveLength(6);
expect(parts.every(Number.isInteger)).toBe(true);
expect(parts.reduce((a,b)=>a+b,0)).toBe(32);
expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);

Adding these ensures the test suite verifies all required behaviors, not just this single example match.

Comment thread src/splitInteger.test.js Outdated
Comment on lines +17 to +18
expect(splitInteger(17, 0))
.toEqual([]);
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 invalid given the task's input assumptions and the test title. It calls splitInteger(17, 0) (zero parts) but the description states inputs are positive integers and the test title says "should add zeros if value < numberOfParts". numberOfParts should not be 0. Replace this test with a proper "value < numberOfParts" case, for example splitInteger(2, 5) which should return [0,0,0,1,1] (sorted) and satisfy max - min <= 1 and sum equality. Also add assertions for length and integer elements.

Suggested replacement:

const parts = splitInteger(2, 5);
expect(parts).toEqual([0, 0, 0, 1, 1]);
expect(parts).toHaveLength(5);
expect(parts.reduce((a,b)=>a+b,0)).toBe(2);
expect(parts.every(Number.isInteger)).toBe(true);
expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);

This aligns the test input with the task requirements and the test title.

Zmieniono testy w pliku src/splitInteger.test.js:
- poprawiono przypadek z 17,4 na 6,2
- dodano sprawdzenia sumy, długości i różnicy max-min
- poprawiono przypadek value < numberOfParts
Copy link
Copy Markdown

@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.

Review Summary

I am requesting changes because the test suite is missing required assertions and example cases from the specification, which are core requirements and block full compliance.

Critical issues to fix:

  • In src/splitInteger.test.js, add an explicit ascending-order assertion to each existing test after the last current expect. For example: expect(parts.every((v, i) => i === 0 || parts[i] >= parts[i - 1])).toBe(true). Apply this to the tests for splitInteger(6, 2), splitInteger(32, 6), and the value < numberOfParts case (e.g., [0, 0, 0, 1, 1]).
  • Add two missing example tests required by the task description: (1) splitInteger(8, 1) should toEqual([8]) and include checks for length, sum, integer elements, sortedness, and max - min <= 1; (2) splitInteger(17, 4) should toEqual([4, 4, 4, 5] with the same checks. Place these after the existing tests.

Why this matters: the spec mandates that results are sorted non-decreasing and includes these examples explicitly; without these, the suite doesn’t fully validate the requirement.

Minor suggestion (optional): consider a small helper to DRY common assertions (length, sum, integers, sortedness, max-min).

You’ve done solid work covering length, sum, integer elements, and max-min; with the sortedness checks and the two example tests added, this will be ready to approve.


✨ 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/splitInteger.test.js
expect(parts).toHaveLength(2);
expect(parts.reduce((a, b) => a + b, 0)).toBe(6);
expect(parts.every(Number.isInteger)).toBe(true);
expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add an explicit assertion that the returned array is sorted in ascending (non-decreasing) order for this test. The spec requires the array to be sorted even though other properties (sum, integer elements, max-min) are already checked. Example assertion to add after the existing expects:

expect(parts.every((v, i) => i === 0 || parts[i] >= parts[i - 1])).toBe(true);

This should be inserted after the last expect in this test so sorting is verified for splitInteger(6, 2).

Comment thread src/splitInteger.test.js
expect(parts).toHaveLength(6);
expect(parts.reduce((a, b) => a + b, 0)).toBe(32);
expect(parts.every(Number.isInteger)).toBe(true);
expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add the explicit sortedness assertion here as well. The test already checks toEqual([5, 5, 5, 5, 6, 6]), length, sum and max-min <= 1 — but the task requires a dedicated ascending-order check. Insert the same every((v,i) => ... ) check after the existing expects in this test for splitInteger(32, 6).

Comment thread src/splitInteger.test.js
expect(parts).toHaveLength(5);
expect(parts.reduce((a, b) => a + b, 0)).toBe(2);
expect(parts.every(Number.isInteger)).toBe(true);
expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add the explicit sortedness assertion for this test too. Although you assert the exact array [0, 0, 0, 1, 1], include the dedicated ascending-order check so the test suite matches the spec. Place the check after the current expects in this value < numberOfParts test.

Comment thread src/splitInteger.test.js
Comment on lines +15 to +20
const parts = splitInteger(32, 6);
expect(parts).toEqual([5, 5, 5, 5, 6, 6]);
expect(parts).toHaveLength(6);
expect(parts.reduce((a, b) => a + b, 0)).toBe(32);
expect(parts.every(Number.isInteger)).toBe(true);
expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add two missing example tests required by the task description: 1) splitInteger(8, 1) must toEqual([8]) plus asserts for length, sum, integer elements, sortedness, and max - min <= 1; 2) splitInteger(17, 4) must toEqual([4, 4, 4, 5]) plus the same properties. Insert these new tests (for example) after the existing tests so all examples from the description are covered. See the task description for the exact expected outputs.

Dodano brakujące testy dla przykładów z opisu zadania, 
sprawdzenie sortowania oraz wszystkich wymagań: suma, długość, liczby całkowite, max-min <= 1.
Copy link
Copy Markdown

@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.

Review Summary

Your solution is approved. You correctly included the required example cases and validated all key properties: exact length, integer elements, sum equals the input value, non-decreasing order, and max–min ≤ 1. Nice job also adding an edge case where the value is smaller than the number of parts—this shows good test thinking.

Educational override note: While approving, I want to highlight a couple of minor improvements to help you grow. In src/splitInteger.test.js, the repeated assertions across tests can be refactored into a small helper (e.g., assertParts(value, numberOfParts, parts, expected)) to reduce duplication and improve maintainability. Also, consider adding a brief comment in src/splitInteger.js explaining the floor-distribution approach and why some parts are incremented by 1—this boosts readability. As a tiny bonus, a simple sanity test like splitInteger(1, 1) further solidifies coverage.

Great progress—clean, correct, and easy to follow work!


✨ 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