Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current tests for src/reduce.test.js do not yet fully cover the core MDN behaviors required by the task. You correctly test basic summation with and without an initial value, and the setup/teardown that attaches reduce2 to Array.prototype is solid, but several critical behaviors are still untested. Most importantly, there is no test that [].reduce2(callback) (no initial value) throws a TypeError, which is required MDN behavior and a core requirement of the assignment.
Here are the key issues to fix:
- Add a test that uses
jest.fnas the callback and asserts that it is called once per array element, with the correct arguments(accumulator, currentValue, index, array)and correct call count (e.g., for[1,2,3]). - For the test without an initial value, use a spy callback and assert that the first call starts at index
1(matching MDN semantics when no initial value is provided). - Add empty-array tests: (1) for
[].reduce2(fn, initialValue)returning the initial value, and (2) for[].reduce2(fn)throwing aTypeError. - Add a test for sparse arrays (e.g.,
const arr = [1, , 3];) asserting that the callback is not called for the missing index.
These adjustments will bring your tests in line with the MDNreducespecification and make your suite much more robust; you are close, and tightening these edge cases will significantly improve the quality of your 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
| }); | ||
|
|
||
| it('should ', () => { | ||
| it('should call callback for each array element', () => { |
There was a problem hiding this comment.
The test name says it should call the callback for each array element, but the test only asserts the final sum. Use a spy (const cb = jest.fn((acc, curr, idx, arr) => acc + curr)) and assert call count and the arguments passed (index and array). This verifies the callback invocation and parameters as required by MDN.
| it('uses first element as initial value if startValue is omitted', () => { | ||
| const array = [9, 2, 6, 6]; | ||
|
|
||
| const result = array.reduce2((acc, curr) => acc + curr); |
There was a problem hiding this comment.
The test that omits the initial value correctly checks the numeric result, but you should also assert that the callback starts from index 1 (when no initial value) by using a spy and checking the first call's index argument. That ensures the implementation matches the MDN behavior for initial-value omission.
|
|
||
| const result = array.reduce2((acc, curr) => acc + curr); | ||
|
|
||
| expect(result).toBe(5); |
There was a problem hiding this comment.
Add tests for empty-array behavior: (1) when an initial value is provided, reduce2 should return that initial value for an empty array; (2) when no initial value is provided on an empty array, reduce2 must throw a TypeError. These are required behaviors from the MDN spec and must be covered.
| const result = array.reduce2((acc, curr) => acc + curr); | ||
|
|
||
| expect(result).toBe(5); | ||
| }); |
There was a problem hiding this comment.
Consider adding a test for sparse arrays (arrays with holes) to confirm that holes are skipped (i.e., the callback is not called for missing indices). This is a subtle but important part of matching native Array.prototype.reduce semantics.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because two MDN-required behaviors for reduce2 are still not correctly covered by your tests, and these are considered critical for this task. First, you need an explicit test that calling [].reduce2(cb) without an initial value throws a TypeError; for example, near your existing empty-array test in src/reduce.test.js, add something like:
expect(() => [].reduce2(cb)).toThrow(TypeError);This verifies the mandated empty-array/no-initial-value error path. Second, the sparse-array test currently expects the callback to be called for the hole and to produce NaN, but per MDN semantics holes must be skipped, meaning your jest mock should only be called for existing indices (e.g., indices 0 and 2) and the final result should reflect that (for [1, , 3] with initial 0, result should be 4). Please update that test to assert the callback call count and arguments only for present elements and remove any expectation that the callback is called with undefined for the missing index. Once these two issues are fixed, the rest of your reduce2 tests look solid and will give you much better coverage of edge cases; you’re very close, and these adjustments will align your suite with the official spec.
✨ 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
| it('returns initial value when array is empty', () => { | ||
| const array = []; | ||
| const cb = jest.fn((acc, curr) => acc + curr); | ||
|
|
||
| const result = array.reduce2(cb, 2); | ||
|
|
||
| expect(result).toBe(2); | ||
| expect(cb).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Missing test: add a test asserting that calling [].reduce2(cb) (no initial value) throws a TypeError. For example:
expect(() => [].reduce2(cb)).toThrow(TypeError);Add this alongside the existing empty-array test so both "with initial value" and "without initial value" behaviors are covered (MDN-required).
| it('skips holes in sparse arrays', () => { | ||
| const array = new Array(3); | ||
|
|
||
| array[0] = 1; | ||
| array[2] = 3; | ||
|
|
||
| const cb = jest.fn((acc, curr) => acc + curr); | ||
|
|
||
| const result = array.reduce2(cb, 0); | ||
|
|
||
| expect(result).toBeNaN(); | ||
|
|
||
| expect(cb).toHaveBeenCalledTimes(3); | ||
| expect(cb).toHaveBeenNthCalledWith(1, 0, 1, 0, array); | ||
| expect(cb).toHaveBeenNthCalledWith(2, 1, undefined, 1, array); | ||
| expect(cb).toHaveBeenNthCalledWith(3, NaN, 3, 2, array); | ||
| }); |
There was a problem hiding this comment.
The sparse-array test is incorrect. It currently expects the callback to be called for the hole and the result to be NaN. According to the MDN spec, holes must be skipped — the callback should not be called for the missing index. Update the test so it asserts the callback is called only for present elements (for your array with values at indices 0 and 2 the callback should be called twice) and assert the correct final result (e.g. expect(result).toBe(4) for initial 0). Remove the assertion that a call occurred with currentValue undefined for the hole.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you refine the remaining edge cases. You did a solid job covering many core reduce2 behaviors in src/reduce.test.js, including using the first element as the default accumulator, handling single-element arrays, and correctly throwing a TypeError for [].reduce2(cb) without an initial value. The main correction needed in src/reduce.test.js is the sparse-array test: for const array = [1, , 3] with initial 0, the callback should only be called for indices 0 and 2, and the final result should be 4 (no call for the hole at index 1). In src/reduce.js, you correctly throw a TypeError for empty arrays without an initial value, but you still need to (1) validate that callback is a function near the top of reduce2, (2) when no initial value is provided, find the first present element to use as the accumulator and throw if the array has only holes, and (3) skip holes during iteration with a check like if (!(i in this)) continue;. These changes will align your implementation with MDN/ECMAScript behavior for sparse arrays and invalid callbacks. Overall, your progress is strong and you’re very close to a spec-accurate implementation—treat these adjustments as a chance to sharpen your understanding of how reduce really behaves under the hood.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| it('skips holes in sparse arrays', () => { | ||
| const array = new Array(3); | ||
|
|
||
| array[0] = 1; | ||
| array[2] = 3; | ||
|
|
||
| const cb = jest.fn((acc, curr) => acc + curr); | ||
|
|
||
| const result = array.reduce2(cb, 0); | ||
|
|
||
| expect(result).toBeNaN(); | ||
|
|
||
| expect(cb).toHaveBeenCalledTimes(3); | ||
| expect(cb).toHaveBeenNthCalledWith(1, 0, 1, 0, array); | ||
| expect(cb).toHaveBeenNthCalledWith(2, 1, undefined, 1, array); | ||
| expect(cb).toHaveBeenNthCalledWith(3, NaN, 3, 2, array); |
There was a problem hiding this comment.
The sparse-array test claims to “skip holes” but the assertions expect three calls (including an invocation with undefined) and NaN as the result. According to MDN semantics, array holes must be skipped. Update this test so the callback is only called for present elements (indices 0 and 2) and the final result for array = [1, , 3] with initial 0 is 4. Example expectations to use instead:
expect(result).toBe(4);expect(cb).toHaveBeenCalledTimes(2);expect(cb).toHaveBeenNthCalledWith(1, 0, 1, 0, array);expect(cb).toHaveBeenNthCalledWith(2, 1, 3, 2, array);
| it('throws TypeError for empty array without initial value', () => { | ||
| const array = []; | ||
| const cb = jest.fn(); | ||
|
|
||
| expect(() => array.reduce2(cb)).toThrow(TypeError); |
There was a problem hiding this comment.
Good: there is an explicit test that [].reduce2(cb) without an initial value throws a TypeError — this satisfies the required empty-array/no-initial-value error path required by the spec and previous feedback.
| if (this.length === 0 && arguments.length < 2) { | ||
| throw new TypeError('Reduce of empty array with no initial value'); |
There was a problem hiding this comment.
Good that you throw a TypeError for empty arrays without an initial value, but this check alone is not enough. If the array has length > 0 but all indexes are holes (sparse with no present elements) and no initial value was provided, the method should also throw a TypeError. Consider, when no initial value is provided, searching for the first present index and using that element as the accumulator; if none found, throw a TypeError.
| if (this.length === 0 && arguments.length < 2) { | ||
| throw new TypeError('Reduce of empty array with no initial value'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Before using callback you should validate it's callable and throw a TypeError otherwise (per the spec). Add something like if (typeof callback !== 'function') throw new TypeError('callback is not a function'); near the start of the function.
No description provided.