Skip to content

solution#1026

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

solution#1026
webEsperer wants to merge 3 commits into
mate-academy:mainfrom
webEsperer:develop

Conversation

@webEsperer
Copy link
Copy Markdown

No description provided.

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

Decision: REQUEST_CHANGES

Your tests cover several representative cases (even split, single part, uneven split, and value < parts), which is great. However, they do not assert the full contract required by the task, which is a critical requirement for completion.

Critical issues to fix (blockers):

  • In src/splitInteger.test.js, add explicit assertions for the required properties on the result for each case:
    • length equals numberOfParts (e.g., expect(parts).toHaveLength(n))
    • all elements are integers (e.g., expect(parts.every(Number.isInteger)).toBe(true))
    • array is sorted ascending (e.g., expect(parts).toEqual([...parts].sort((a,b)=>a-b)))
    • max - min <= 1 (e.g., expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1))
    • sum equals value (e.g., expect(parts.reduce((s,x)=>s+x,0)).toBe(value)).
  • Ensure these property checks are present for the uneven and edge cases too, not only the even split example.

Minor improvement:

  • In .eslintrc.js, prefer 'off' instead of 0 for the no-proto rule, and add a brief comment if disabling is intentional since __proto__ usage is discouraged.

Once these assertions are added, your suite will robustly verify the full contract. Nice job on the representative scenarios—augmenting them with the property checks will complete the 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

Comment thread .eslintrc.js Outdated
'no-proto': 0,
strict: 0,
'max-len': 0,
'no-proto': 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 rule is disabled using the numeric level 0. It's valid but less readable than the named level. Consider using 'off' for clarity, and double-check whether disabling no-proto is necessary — manipulating __proto__ is usually discouraged. If disabling is intentional, add a short comment explaining why.

Comment thread src/splitInteger.test.js
test(`should split a number into equal parts
if a value is divisible by a numberOfParts`, () => {

test(`should split a number into equal parts
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 name and example here are fine, but this test (and others) only assert exact array equality for one example. According to the requirements you must also verify the general properties of the result: - the returned array length equals numberOfParts - all elements are integers - the array is sorted ascending - max - min <= 1 - the sum of elements equals value

Consider augmenting this test (or adding a separate test) to assert those properties explicitly. Example assertions you could add (conceptually):

  • expect(parts).toHaveLength(5)
  • expect(parts.every(Number.isInteger)).toBe(true)
  • expect(parts).toEqual(parts.slice().sort((a,b)=>a-b))
  • expect(Math.max(...parts) - Math.min(...parts)).toBeLessThanOrEqual(1)
  • expect(parts.reduce((s,x)=>s+x,0)).toBe(10)

This will ensure the tests check the full contract instead of only the example output.

Comment thread src/splitInteger.test.js Outdated

test(`should split a number into equal parts
if value is divisible by numberOfParts`, () => {
expect(splitInteger(10, 5)).toEqual([2, 2, 2, 2, 2]);
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 expectation checks the explicit example [2,2,2,2,2], which is important, but please add assertions for the generic properties listed in the task description (length, integers, sorted, max-min <= 1, sum equals value). Relying only on equality to a literal array does not prove those properties for other inputs.

Comment thread src/splitInteger.test.js Outdated

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

expect(splitInteger(10, 3)).toEqual([3, 3, 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 for an uneven split uses a concrete expected array [3, 3, 4], which is good. The requirements also expect that when values differ they differ by at most 1 and that larger values appear at the end (sorted ascending). Add explicit assertions for sorting and for max - min <= 1 so the test documents and checks those contract rules rather than only the example.

Comment thread src/splitInteger.test.js Outdated

test('should add zeros if value < numberOfParts', () => {

expect(splitInteger(2, 3)).toEqual([0, 1, 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.

Good that you included the value < numberOfParts case. In addition to checking the exact expected array, add assertions for the general properties (length, integerness, sorted order, max-min <= 1, sum equals value) so the test suite formally verifies behavior for this edge case as required by the task.

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

Decision: REQUEST_CHANGES

Your tests are well-structured and the helper assertSplitProperties thoroughly validates the contract. However, a core requirement is missing: the suite must include the four explicit example cases from the task description. This is a critical blocker because those exact outputs must be asserted to meet the specification.

Critical items to fix:

  • Add explicit tests after the existing ones (around line ~60) for:
    • splitInteger(8, 1) → expect [8]
    • splitInteger(6, 2) → expect [3, 3]
    • splitInteger(17, 4) → expect [4, 4, 4, 5]
    • splitInteger(32, 6) → expect [5, 5, 5, 5, 6, 6]
  • For each, also call assertSplitProperties(parts, value, numberOfParts) to keep the property checks.

Minor (non-blocking) suggestions:

  • In .eslintrc.js, add a brief inline comment explaining why no-proto is disabled (e.g., tests use __proto__ for legacy mocking).

Once those four example tests are added, the suite will fully satisfy the requirements. Nice work on the comprehensive property-based checks!


✨ 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 .eslintrc.js
'no-proto': 0,
strict: 0,
'max-len': 0,
'no-proto': 'off'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider adding a brief comment explaining why the no-proto rule is disabled (for example, if some dependency or test utilities intentionally use __proto__). This helps future maintainers understand the rationale for turning the rule off.

Example (inline comment on this line):

// disable because tests use __proto__ for mocking legacy behavior
'no-proto': 'off'

No functional change required — this is a documentation suggestion to improve maintainability.

Comment thread src/splitInteger.test.js
const parts = splitInteger(value, numberOfParts);

expect(parts).toEqual([0, 1, 1]);
assertSplitProperties(parts, value, numberOfParts);
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 explicit tests for the concrete examples required by the task description. The description mandates 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] . You already have the helper assertSplitProperties(parts, value, numberOfParts) that checks all required properties — reuse it for these cases.

Place the tests after the existing ones (for example, after the last assertion around line 60). Example tests you can add (adapt to your style):

test('example: splitInteger(8, 1)', () => {
  const value = 8;
  const numberOfParts = 1;
  const parts = splitInteger(value, numberOfParts);

  expect(parts).toEqual([8]);
  assertSplitProperties(parts, value, numberOfParts);
});

test('example: splitInteger(6, 2)', () => {
  const value = 6;
  const numberOfParts = 2;
  const parts = splitInteger(value, numberOfParts);

  expect(parts).toEqual([3, 3]);
  assertSplitProperties(parts, value, numberOfParts);
});

test('example: splitInteger(17, 4)', () => {
  const value = 17;
  const numberOfParts = 4;
  const parts = splitInteger(value, numberOfParts);

  expect(parts).toEqual([4, 4, 4, 5]);
  assertSplitProperties(parts, value, numberOfParts);
});

test('example: splitInteger(32, 6)', () => {
  const value = 32;
  const numberOfParts = 6;
  const parts = splitInteger(value, numberOfParts);

  expect(parts).toEqual([5, 5, 5, 5, 6, 6]);
  assertSplitProperties(parts, value, numberOfParts);
});

After adding these four tests the suite will explicitly assert the required examples and the existing property checks will ensure the rest of the contract is enforced.

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

APPROVE

Great work—your test suite thoroughly verifies the key properties of splitInteger (length, integer elements, non-decreasing order, max-min ≤ 1, and sum), and it includes the four required example scenarios. One small, educational override note so you can keep progressing: please add an explicit test for the edge case where value === numberOfParts (e.g., place it after the test ending around line 92 in src/splitInteger.test.js). This aligns with the task requirement and makes the intent crystal clear; for example, assert [1, 1, 1, 1, 1] for splitInteger(5, 5) and run assertSplitProperties to confirm.

Minor clarity improvement: in .eslintrc.js, add a brief inline comment next to the disabled no-proto rule to explain why it’s off (e.g., legacy tests using __proto__). These are straightforward refinements; overall, you’ve implemented the core checks well—nice job keeping the tests clean and focused.


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

Footnotes

  1. Rate AI review example

Comment thread .eslintrc.js
'no-proto': 0,
strict: 0,
'max-len': 0,
'no-proto': 'off'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please add a brief inline comment explaining why no-proto is disabled. For example:

// Disabled because some tests use `__proto__` for legacy mocking
'no-proto': 'off'

This is a non-blocking clarity improvement so maintainers understand the rationale for the rule override.

Comment thread src/splitInteger.test.js
const parts = splitInteger(value, numberOfParts);

expect(parts).toEqual([5, 5, 5, 5, 6, 6]);
assertSplitProperties(parts, value, numberOfParts);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please add an explicit test for the case where value === numberOfParts (this edge case was requested in the task guidance). Place it after the existing examples (for example, after the test ending on line 92). Example test to add:

test('should split when value equals numberOfParts', () => {
  const value = 5;
  const numberOfParts = 5;
  const parts = splitInteger(value, numberOfParts);

  expect(parts).toEqual([1, 1, 1, 1, 1]);
  assertSplitProperties(parts, value, numberOfParts);
});

This ensures the suite explicitly covers the "value equals numberOfParts" scenario required by the implementation details.

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