Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've done a great job creating a comprehensive test suite for the reduce function, covering many important scenarios. However, changes are needed for two tests that don't align with the standard behavior of Array.prototype.reduce.
- Empty array without initial value: Your test expects
undefined, but the specification requires aTypeErrorto be thrown in this case. Please update the test insrc/reduce.test.jsto assert that an error is thrown, for example by using.toThrow(TypeError). - Sparse arrays: The native
reducemethod skips empty slots in sparse arrays. Your test should be updated to validate this correct behavior, ensuring the callback is only invoked for defined elements.
Correcting these edge cases is crucial for ensuring your test suite accurately validates any reduce implementation. You're very close to a perfect 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
src/reduce.test.js
Outdated
| test('throws TypeError on empty array without initial value', () => { | ||
| expect(callReduce([], (acc, v) => acc + v)).toBeUndefined(); |
There was a problem hiding this comment.
According to the MDN documentation, calling reduce on an empty array without an initial value should throw a TypeError. This test incorrectly expects the result to be undefined. To fix this, you should wrap the call in a function and use .toThrow(TypeError) to assert that an error is thrown.
src/reduce.test.js
Outdated
| }); | ||
|
|
||
| // Add tests here | ||
| test('skips holes in sparse arr(your implementation DOES NOT skip)', () => { |
There was a problem hiding this comment.
This test is written to pass for a faulty implementation, but a test should always assert the correct, specified behavior. According to the reduce specification, empty slots in a sparse array should be skipped.
A better approach is to write a test that checks for this correct behavior. For example:
test('skips empty slots in a sparse array', () => {
const arr = [1, , 3]; // A sparse array
const callback = jest.fn((sum, el) => sum + el);
const result = callReduce(arr, callback, 0);
expect(callback).toHaveBeenCalledTimes(2);
expect(result).toBe(4); // 0 + 1 + 3
});This way, the test will fail if the implementation is incorrect, which is the purpose of testing.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on these tests! You've correctly handled the sparse array case, which is excellent.
However, this requires one small but critical change. In src/reduce.test.js, the test that asserts a TypeError is thrown for an empty array is calling a function named reduce2 instead of your implementation.
// src/reduce.test.js
it('throws a TypeError if the array is empty and no initial value is provided', () => {
expect(() => reduce2([], (a, b) => a + b)).toThrow(TypeError);
});Because reduce2 isn't defined, this test doesn't actually validate your reduce function's behavior for this important edge case. Please update it to use the callReduce helper to ensure your implementation is being tested correctly. 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
src/reduce.test.js
Outdated
| afterAll(() => { | ||
| delete Array.prototype.reduce2; | ||
| test('throws TypeError on empty array without initial value', () => { | ||
| expect(() => [].reduce2((acc, v) => acc + v)).toThrow(TypeError); |
There was a problem hiding this comment.
This test correctly asserts that a TypeError should be thrown, which is great! However, you're calling a function named reduce2 which doesn't seem to be the function under test. You should use the callReduce helper to test your custom reduce implementation with an empty array.
| return prev; | ||
| return accumulator; | ||
| } | ||
|
|
There was a problem hiding this comment.
You're correctly testing that a TypeError should be thrown, but it looks like you're calling a function named reduce2 which isn't defined. You should use your callReduce helper here to ensure you're testing your custom reduce function.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! Your implementation of the custom reduce function is solid, and it correctly handles various edge cases. I'm particularly impressed with how you addressed the feedback from the previous review by correcting the test for an empty array. The test suite is now comprehensive and effectively validates your implementation. This submission meets all the requirements, and I am happy to approve it. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.