my solution by Serhii-Khobotov · Pull Request #1013 · mate-academy/js_split_integer

Serhii-Khobotov · 2025-09-05T07:39:13Z

No description provided.

mateacademy-ai-mentor

Review Summary

Decision: REQUEST_CHANGES

Your Jest tests for splitInteger are clear and cover some core behaviors, but several critical, spec-mandated properties are not explicitly tested. This can allow incorrect implementations to pass, so changes are required before approval.

Critical issues to fix (blockers):

Add explicit assertions that the result is always an array and has length numberOfParts (e.g., Array.isArray(result) and result).toHaveLength(numberOfParts)).
Verify all elements are integers in every case (e.g., result.every(Number.isInteger)).
Verify the sum of all parts equals value (e.g., result.reduce((a,b)=>a+b,0) === value).
Assert the array is sorted in non-decreasing order (e.g., compare to a sorted copy or check result[i] <= result[i+1]).
Assert the difference between max and min is at most 1 (e.g., Math.max(...result) - Math.min(...result) <= 1).
Verify distribution counts when not evenly divisible: number of larger elements should be value % numberOfParts, and smaller elements numberOfParts - (value % numberOfParts) (with values q and q+1, where q = Math.floor(value/numberOfParts)).
Add tests for all specified examples from the requirements: splitInteger(8, 1), splitInteger(6, 2), splitInteger(17, 4), splitInteger(32, 6).

Why this matters: The specification requires these properties for every output. Without explicit checks, an implementation could return floats, unsorted arrays, wrong lengths, or sums that still pass equality-based example tests.

Where to adjust (examples):

In src/splitInteger.test.js, after each call to splitInteger(value, numberOfParts), add property checks listed above. Consider creating a helper that asserts these properties to reuse across tests.
Replace implicit ordering checks (exact array equality) with explicit ascending-order checks to guard against correct values in the wrong order.

Minor improvements (nice-to-have):

Use parameterized tests (test.each) to cover many cases succinctly.
Improve test names to reflect properties being validated (e.g., “ensures sum equals value”).

You’re on the right track—the existing cases like (12, 3), (7, 1), (8, 3), and (3, 5) are good. Add the explicit property-based assertions and the missing example cases, and this will be ready to approve. Keep going!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor

Review Summary

Decision: REQUEST_CHANGES

Your implementation in src/splitInteger.js looks correct and aligns with the requirements. However, there are critical blockers in src/splitInteger.test.js that conflict with the spec and can lead to incorrect or misleading test results.

Critical issues to fix

Invalid test inputs (positive integers required):
- Lines with numberOfParts = 0 (e.g., splitInteger(5, 0), splitInteger(17, 0)) must be removed. These violate the requirement that inputs are valid positive integers.
- Lines where numberOfParts > value (e.g., splitInteger(3, 5), splitInteger(3, 6), splitInteger(5, 10)) should be removed. The spec doesn’t require handling such cases, and examples never produce zeros.
- Why: The task specifies only valid positive integers and does not require testing error handling or zero-padding outputs. Keeping these tests may fail correct implementations.
- How: Delete these test cases or adjust them so numberOfParts <= value and both arguments are positive.
Incorrect use of Math.max/Math.min:
- Lines calling Math.max(splitInteger(...)) and Math.min(splitInteger(...)) should use the spread operator: Math.max(...splitInteger(...)) and Math.min(...splitInteger(...)).
- Also ensure both max and min use the same call, e.g., splitInteger(17, 1) for both.
- Why: Math.max/Math.min expect a list of numbers, not an array. Without spreading, results are incorrect.
- How: Update those assertions accordingly and remove any associated cases with numberOfParts = 0.

Minor improvements (optional)

Simplify/clarify some test names for readability.
Consider small helpers for repeated checks (e.g., sorted order) to make tests more maintainable.

Once you remove the invalid tests and fix the Math.max/Math.min usage, your suite will fully comply with the requirements. Nice work on the core function and the breadth of valid test coverage—you're very close!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2025-09-05T08:59:09Z

src/splitInteger.test.js

 });

 test('should add zeros if value < numberOfParts', () => {
+  expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]);


This test expects zeros in the output for splitInteger(3, 5), but the requirements specify that inputs are always valid positive integers and the examples never produce zeros. You should remove this test and any similar ones, as they do not align with the task requirements (see 1.10 and 2.2).

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js


 test('should add zeros if value < numberOfParts', () => {
+  expect(splitInteger(3, 5)).toEqual([0, 0, 1, 1, 1]);
+  expect(splitInteger(3, 6)).toEqual([0, 0, 0, 1, 1, 1]);


Same as above: this test expects zeros in the output for splitInteger(3, 6), which is not required by the spec. Remove this test.

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js

+});
+
+test('should always return an array of length numberOfParts', () => {
+  expect(splitInteger(5, 10)).toHaveLength(10);


This test checks for array length when numberOfParts is greater than value (e.g., splitInteger(5, 10)), which is not a valid input according to the requirements. Remove or adjust this test to only use valid positive integers where numberOfParts <= value.

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js

+});
+
+test('should always return an array if numberOfParts 0', () => {
+  expect(splitInteger(5, 0)).toBeInstanceOf(Array);


This test checks for array output when numberOfParts is zero, which is not a valid input according to the requirements. Remove this test.

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js

+test('should return an array with integer in every case', () => {
+  expect(splitInteger(5, 3).every(Number.isInteger)).toBe(true);
+  expect(splitInteger(5, 10).every(Number.isInteger)).toBe(true);
+  expect(splitInteger(5, 0).every(Number.isInteger)).toBe(true);


This test checks for integer elements when numberOfParts is zero, which is not a valid input according to the requirements. Remove this test.

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js

+
+test('should deference between max and min part to be <= 1', () => {
+  if (splitInteger().length > 0) {
+    expect(Math.max(splitInteger(17, 3)) - Math.min(splitInteger(17, 3)))


This test checks the max-min difference using Math.max(splitInteger(...)), which is incorrect. Math.max expects a list of numbers, not an array. Use the spread operator: Math.max(...splitInteger(...)). Also, remove any cases with numberOfParts = 0.

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js

+    expect(Math.max(splitInteger(17, 3)) - Math.min(splitInteger(17, 3)))
+      .toBeLessThanOrEqual(1);
+
+    expect(Math.max(splitInteger(17, 10)) - Math.min(splitInteger(17, 10)))


Same as above: use the spread operator for Math.max/Math.min.

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js

+    expect(Math.max(splitInteger(17, 10)) - Math.min(splitInteger(17, 10)))
+      .toBeLessThanOrEqual(1);
+
+    expect(Math.max(splitInteger(17, 4)) - Math.min(splitInteger(17, 1)))


This test uses splitInteger(17, 1) as the second argument to Math.min, which is incorrect. It should be splitInteger(17, 1) for both max and min, and use the spread operator. Also, remove any cases with numberOfParts = 0.

mateacademy-ai-mentor · 2025-09-05T08:59:10Z

src/splitInteger.test.js

+    expect(Math.max(splitInteger(17, 4)) - Math.min(splitInteger(17, 1)))
+      .toBeLessThanOrEqual(1);
+
+    expect(Math.max(splitInteger(17, 0)) - Math.min(splitInteger(17, 0)))


This test uses splitInteger(17, 0), which is not a valid input. Remove this test.

mateacademy-ai-mentor

Review Summary

Educational approval: you’re on the right track, and I’m approving this to support your progress. Great job implementing a concise, correct greedy split in src/splitInteger.js that returns ascending parts summing to value with max–min ≤ 1.

Critical fixes to address next:

src/splitInteger.test.js: Remove the unintended guard at the max/min test (around the block titled "should deference…"): if (splitInteger().length > 0) { ... }. It calls splitInteger() with no args and skips the assertions; per the requirement, those checks must run directly on concrete inputs (e.g., 17, 3, 17, 10, 17, 4).
Lint failure (max-len): Break long lines by storing results in variables, e.g.:
- const parts = splitInteger(17, 3); const diff = Math.max(...parts) - Math.min(...parts); expect(diff).toBeLessThanOrEqual(1);
- For sorted checks: const parts32 = splitInteger(32, 6); expect(parts32).toEqual(parts32.slice().sort((a, b) => a - b));

Minor improvements:

Fix the typo in the test title: “deference” → “difference”.
In src/splitInteger.js Line ~23, remove the commented debug line // console.log(splitInteger(12, 0)); and the stray blank line below it.
Prefer storing const parts = splitInteger(...) once per test to avoid repeated calls and improve clarity, especially for cases like splitInteger(5, 10); keep that case if you intend to support numberOfParts > value and ensure it yields balanced zeros/ones.

You’ve met the core logic requirements; with these tidy-ups and the test guard removal, your suite will robustly enforce the spec. Keep up the solid work!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2025-09-11T09:57:00Z

src/splitInteger.js

  return parts;
 }

+// console.log(splitInteger(12, 0));


Remove the commented-out debugging line // console.log(splitInteger(12, 0));. It is unnecessary in production/test code and may confuse reviewers. The function already exports correctly, so the log can be deleted. (Line currently commented out.)

mateacademy-ai-mentor · 2025-09-11T09:57:00Z

src/splitInteger.js

 }

+// console.log(splitInteger(12, 0));
+


Remove the extra blank/commentable marker line. Keeping stray blank/commentable lines is harmless but removing it keeps the file tidy.

mateacademy-ai-mentor · 2025-09-11T09:57:00Z

src/splitInteger.test.js


-test('should add zeros if value < numberOfParts', () => {
+test('should deference between max and min part to be <= 1', () => {
+  if (splitInteger().length > 0) {


Critical: This conditional uses a call with no arguments and causes the assertions inside to be skipped. Calling splitInteger() without parameters is unintended; remove the if guard and run the max/min assertions directly for concrete inputs (e.g., splitInteger(17, 3)). This test is intended to ensure the difference between max and min is <= 1 and must not be bypassed. See the task requirements about max-min <= 1.

mateacademy-ai-mentor · 2025-09-11T09:57:00Z

src/splitInteger.test.js

 });

-test('should add zeros if value < numberOfParts', () => {
+test('should deference between max and min part to be <= 1', () => {


Minor: Typo in test title — "deference" should be "difference". Fixing this improves clarity of the test description but does not affect behavior.

mateacademy-ai-mentor · 2025-09-11T09:57:00Z

src/splitInteger.test.js

+
+test('should return an array with integer in every case', () => {
+  expect(splitInteger(5, 3).every(Number.isInteger)).toBe(true);
+  expect(splitInteger(5, 10).every(Number.isInteger)).toBe(true);


Suggestion: This assertion uses splitInteger(5, 10) which is a valid input (numberOfParts > value). Keep the test if you intend to support that case, but ensure the implementation returns a balanced distribution (zeros then ones) so every(Number.isInteger) and other expectations hold. If you intended to forbid numberOfParts > value, update the test instead. The task description allows any positive integers.

mateacademy-ai-mentor · 2025-09-11T09:57:00Z

src/splitInteger.test.js

-test('should add zeros if value < numberOfParts', () => {
+test('should deference between max and min part to be <= 1', () => {
+  if (splitInteger().length > 0) {
+    expect(Math.max(...splitInteger(17, 3)) - Math.min(...splitInteger(17, 3)))


Suggestion to make assertions more robust/clear: avoid calling splitInteger(...) multiple times inside the same assertion. Instead store the result in a local variable and reuse it. For example, replace repeated calls in these lines with const parts = splitInteger(17, 3); expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1);. This reduces duplicated calls and makes failures easier to debug.

mateacademy-ai-mentor · 2025-09-11T09:57:01Z

src/splitInteger.test.js

+    expect(Math.max(...splitInteger(17, 3)) - Math.min(...splitInteger(17, 3)))
+      .toBeLessThanOrEqual(1);
+
+    expect(Math.max(...splitInteger(17, 10)) - Math.min(...splitInteger(17, 10)))


Similar suggestion for the other max/min assertions: store const parts = splitInteger(17, 10) and const parts2 = splitInteger(17, 4) before asserting on Math.max/Math.min. This keeps the tests deterministic and clearer.

my solution

6f1ccba

mateacademy-ai-mentor suggested changes Sep 5, 2025

View reviewed changes

added more tests

0d36b4f

Serhii-Khobotov requested a review from mateacademy-ai-mentor September 5, 2025 08:55

mateacademy-ai-mentor suggested changes Sep 5, 2025

View reviewed changes

removed unnecessary cases

2ea5891

Serhii-Khobotov requested a review from mateacademy-ai-mentor September 11, 2025 09:50

mateacademy-ai-mentor approved these changes Sep 11, 2025

View reviewed changes

Conversation

Serhii-Khobotov commented Sep 5, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants