Skip to content

Conversation

@saff-coder
Copy link

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

I have completed all mandatory tasks.

Questions

@saff-coder saff-coder added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 16, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Groups/blob/sprint-1-feedback/Sprint-1/feedback.md

Doing so can help reviewers speed up the review process. Thanks.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 20, 2025
@saff-coder
Copy link
Author

median.js
My implementation now avoids mutating the original array by cloning once before sorting. I use filter to create a new array of numeric values and then use the spread operator ([...numbers]) to create a sorted copy. I don’t create any extra clones beyond those two necessary ones.

dedupe.test.js
In Jest, toEqual checks that two arrays have the same contents, but doesn’t guarantee they are different objects. To check they are different arrays, we should use toBe for identity

Sprint-1/implement/sum.test.js
For the sum tests, I learned that decimal numbers in JavaScript use floating-point arithmetic, which can introduce tiny rounding errors.
That means a direct comparison like:
expect(sum([1.2, 0.6, 0.005])).toEqual(1.805);
might fail even if the function is logically correct.
To handle this, I used Jest’s toBeCloseTo matcher, which is designed for floating-point comparisons:
expect(sum([1.2, 0.6, 0.005])).toBeCloseTo(1.805);

Comment on lines 21 to 22
// 4) Sort a copy numerically (don't mutate the original)
const sorted =[...numbers].sort((a, b) => a - b);
Copy link
Contributor

Choose a reason for hiding this comment

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

numbers is not the original array, and it is not needed afterward.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback!
I removed the unnecessary clone ([...numbers])

Comment on lines +18 to +19
expect(result).toEqual([1, 2, 3, 4]);
expect(result).not.toBe(arr); // important check
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done.

// Given an array with only non-number values
// When passed to the max function
// Then it should return the least surprising value given how it behaves for all other inputs
test.todo("given an array with only non-number values, returns the least surprising value given how it behaves for all other inputs");
Copy link
Contributor

Choose a reason for hiding this comment

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

This test has not been implemented. (You would have to replace the least surprising value by an exact value which you expect)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks so much for spotting those issues!
I went back and fixed the incorrect variable name inside max.js, removed some leftover code from the median exercise, and updated the test that was still calling calculateMedian.
Everything now works as expected, and all tests pass.
Really helpful feedback — thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

The function in this file has has not yet been refactored to meet the spec on line 1.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback!
You're right — I had not refactored the includes function yet.
I've now updated the implementation so that it meets the requirement on line 1 and added tests to confirm it works as intended.

@saff-coder saff-coder requested a review from cjyuan November 20, 2025 15:33
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Changes look good. Well done!

if (numbers.length === 0) return null;

// 4) Sort a copy numerically (don't mutate the original)
const sorted =[...numbers].sort((a, b) => a - b);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also consider introducing a variable with a more meaningful name as

const sorted = numbers.sort((a, b) => a - b);

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants