Skip to content

Develop#1068

Open
Vitalii120296 wants to merge 7 commits intomate-academy:developfrom
Vitalii120296:develop
Open

Develop#1068
Vitalii120296 wants to merge 7 commits intomate-academy:developfrom
Vitalii120296:develop

Conversation

@Vitalii120296
Copy link

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.

Review Summary

I am requesting changes because the implementation is correct, but the test suite does not fully cover the explicit requirements from the task description, which is a functional requirement for this assignment. Your splitInteger function behaves as specified, but in src/splitInteger.test.js you are missing tests for the exact example cases: splitInteger(8, 1) -> [8], splitInteger(6, 2) -> [3, 3], splitInteger(17, 4) -> [4, 4, 4, 5], and splitInteger(32, 6) -> [5, 5, 5, 5, 6, 6] ([CHECKLIST ITEM #4#7]). You also need an explicit property test that checks max - min <= 1 on a non-trivial result, for example:

const result = splitInteger(17, 4);
const diff = Math.max(...result) - Math.min(...result);
expect(diff).toBeLessThanOrEqual(1);

Additionally, add an explicit assertion that the returned array length equals numberOfParts ([CHECKLIST ITEM #1]), e.g. expect(result).toHaveLength(numberOfParts) in at least one test (ideally in those new example tests). Once you add these tests, your suite will directly verify all required properties and example outputs, and your otherwise solid implementation will be fully backed by the required checks. Your core logic looks good, so this is mainly about strengthening and aligning the tests with the stated checklist; you’re very close to a complete solution.


✨ 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


const result = splitInteger(1, 2)

expect(result).toEqual([0, 1])

Choose a reason for hiding this comment

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

Add tests for the exact example cases required by the task description. The suite currently uses other values (10 and 1) but must include: splitInteger(8, 1) -> [8], splitInteger(6, 2) -> [3, 3], splitInteger(17, 4) -> [4, 4, 4, 5], and splitInteger(32, 6) -> [5, 5, 5, 5, 6, 6] (checklist items #4#7). See the description for the exact examples .


const result = splitInteger(10, 3)

expect(result).toEqual([3, 3, 4])

Choose a reason for hiding this comment

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

Add an explicit test that verifies the numeric property max - min <= 1 required by the task (checklist #2). For example: const diff = Math.max(...result) - Math.min(...result); expect(diff).toBeLessThanOrEqual(1) — add this for a non-trivial case (or for each example).


const result = splitInteger(10, 1)

expect(result).toEqual([10])

Choose a reason for hiding this comment

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

Although some toEqual assertions implicitly verify length, add an explicit test asserting the returned array length equals numberOfParts (checklist #1). e.g. expect(result).toHaveLength(numberOfParts) — this makes the intent explicit in the test suite.

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.

Review Summary

I am requesting changes because the test suite is still missing two explicit requirements from the task description, even though the core implementation of splitInteger is correct. First, there is no test for the required example splitInteger(17, 4) -> [4, 4, 4, 5]; this specific case must be asserted with expect(result).toEqual([4, 4, 4, 5]) and expect(result).toHaveLength(4) to fully demonstrate compliance. Second, none of the current tests asserts that the returned array length equals numberOfParts, which is a checklist requirement [CHECKLIST ITEM #1]; please add expect(result).toHaveLength(numberOfParts) in at least one test (for example, the new 17/4 test and/or the existing 6/2 and 32/6 tests). For clarity of the balancing property, it’s also a good idea (and was suggested) to add expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1) to the new 17/4 test so that both a simple and a more complex case explicitly verify the max–min constraint. Once you add the 17/4 test and at least one .toHaveLength(...) assertion, your solution will fully match the stated requirements and can be approved. Overall, the implementation itself is solid—these are just test coverage tweaks to make your solution complete and explicit about the required properties.


✨ 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 on lines +8 to +10
const result = splitInteger(6, 2)

expect(result).toEqual([3, 3])

Choose a reason for hiding this comment

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

Add an explicit length assertion for this test. For example, after const result = splitInteger(6, 2) add expect(result).toHaveLength(2). This satisfies the requirement that the returned array contains exactly numberOfParts elements (checklist item #1).


test('should sort parts ascending if they are not equal', () => {

const result = splitInteger(32, 6)

Choose a reason for hiding this comment

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

The required example splitInteger(17, 4) -> [4, 4, 4, 5] is missing. Add a new test (before or after this test) that asserts the exact array, asserts .toHaveLength(4), and (optionally) checks Math.max(...result) - Math.min(...result) <= 1 so the property is explicit. Example:

test('example: splitInteger(17, 4) -> [4,4,4,5]', () => {
  const result = splitInteger(17, 4)
  expect(result).toEqual([4,4,4,5])
  expect(result).toHaveLength(4)
  expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1)
})

Comment on lines +26 to +27
expect(result).toEqual([5, 5, 5, 5, 6, 6])
expect(diff).toBeLessThanOrEqual(1)

Choose a reason for hiding this comment

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

Consider adding a length assertion to this test as well (e.g. expect(result).toHaveLength(6)) so the suite demonstrates the length-check for a non-trivial case. At minimum, one .toHaveLength(...) must appear in the tests; adding it here makes the property explicit for the 32/6 example.

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.

Review Summary

I am approving your solution as an educational override so you can keep progressing while you learn, even though there is one missing test case to address. Your implementation of splitInteger is solid: it returns the correct number of parts, keeps them balanced (max–min ≤ 1), and outputs them in ascending order, which shows good understanding of the requirements.

The main issue is in src/splitInteger.test.js, where you haven’t yet added the required explicit test for splitInteger(17, 4) producing [4, 4, 4, 5]; this test should also check that the length is 4 and that Math.max(...result) - Math.min(...result) <= 1. To fix this, add a new Jest test block (as suggested in the review comment) right after the related tests, using expect(result).toEqual([4, 4, 4, 5]), expect(result).toHaveLength(4), and the balance assertion. As a minor improvement, you could also add explicit length checks to any tests that don’t have them yet for consistency.

Overall, your core logic and existing tests show strong progress—tightening the test suite with the 17/4 example will make your solution fully aligned with the specification. Keep up the good work; you’re very close to a thoroughly complete solution.


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

Footnotes

  1. Rate AI review example


expect(result).toEqual([5, 5, 5, 5, 6, 6])
expect(result).toHaveLength(6)
expect(diff).toBeLessThanOrEqual(1)

Choose a reason for hiding this comment

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

Add a test for the required example splitInteger(17, 4) === [4, 4, 4, 5]. Place it after this test block. The test should assert the exact array, assert the length equals 4, and verify the balance constraint (max - min <= 1). Example to add:

test('should split 17 into 4 balanced parts', () => {
  const result = splitInteger(17, 4)

  expect(result).toEqual([4, 4, 4, 5])
  expect(result).toHaveLength(4)
  expect(Math.max(...result) - Math.min(...result)).toBeLessThanOrEqual(1)
})

This addresses the missing required example from the description (HIGH priority).

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.

4 participants