West Midlands | 25 Sep ITP | Iswat Bello | Sprint 1 | Module Data Groups #829

Iswanna · 2025-11-10T12:12:15Z

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

This pull request includes modifications to three directories:

Fix directory: I updated the function implementation so that it passes all test cases.

Implement directory: I added test cases based on the provided scenarios and implemented the function to handle all of them.

Refactor directory: I refactored the existing function implementation, ensuring it continues to pass all test cases.

Questions

Hi. Please could you review my PR? I’d really appreciate your feedback.

… about arrays as reference types

This implementation handles arrays with an odd length.

…t mutate the array

…t median for an even-length array

…ulating the median

- Adjusted indentation and arrow function formatting - Added consistent line breaks and spacing - No changes to test logic or order

… to the `validNumbers`

…order

…hen input has no duplicates

… occurrence for arrays of strings and numbers

…ng-number array handling

…turning a new array

cjyuan

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.

Iswanna · 2025-11-20T10:56:54Z

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.

Hi @cjyuan, thank you for sharing the general feedback. I’ll check it against my changes to see whether I’m on the right track.

- Replaced `const copyArray = validNumbers.slice(); const sortArray = copyArray.sort(...)` with `const sorted = [...validNumbers].sort(...)`. - Created a shallow copy and sorts in one step. - Eliminated redundant `copyArray` variable while keeping original array intact.

- Added `expect(result).not.toBe(input)` to tests that check array output. - Confirms that the returned array is a new object, not the same reference as the input. - Maintains `toEqual` checks for content correctness. - Addresses feedback about verifying array copy vs original reference.

- Replaced `toBe` with `toBeCloseTo` for all tests involving decimal numbers. - Ensures floating-point sums are tested correctly despite precision issues. - Updated expected values to match actual sums when necessary (e.g., -7.4 instead of -7.399). - Keeps integer and exact sums using `toBe`.

Iswanna · 2025-11-20T13:21:39Z

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.

Hi @cjyuan, thank you for sharing the general feedback. I’ll check it against my changes to see whether I’m on the right track.

Hi @cjyuan. I have made changes to some files based on the general feedback you shared. Thank you.

cjyuan

Tests are very comprehensive! I am impressed.

cjyuan · 2025-11-20T13:37:04Z

Sprint-1/fix/median.js

+
+  // Create a shallow copy of `validNumbers` and sort it in ascending order
+  // This avoids modifying the original array
+  const sorted = [...validNumbers].sort((a, b) => a - b);


Do we need to clone validNumbers before sorting?

Do we need to clone validNumbers before sorting?

I have gone through the code and I can see that I do not need to clone validNumbers before sorting. I can mutate validNumbers directly since it is already a new array created by .filter(). I have updated the code with these changes.

cjyuan · 2025-11-20T13:41:04Z

Sprint-1/implement/dedupe.test.js

+      const result = dedupe(input);
+      expect(result).toEqual(expected);
+      expect(result).not.toBe(input); // ensures it's a new array


Nicely done.

Nicely done.

Thank you.

cjyuan · 2025-11-20T13:43:19Z

Sprint-1/implement/max.test.js

 // Then it should return the least surprising value given how it behaves for all other inputs
+test("given an array with only non-number values", () => {
+  expect(findMax(["a", null, undefined])).toBe(-Infinity);
+  expect(findMax([{}, [], "hi"]));


Something is missing here

Something is missing here

Thank you for pointing this out. I need to complete the test by adding the expected value.

cjyuan · 2025-11-20T13:44:49Z

Sprint-1/implement/sum.js

+  for (const item of filteredElements) {
+    sumElements += item;
+  }
+  return Number(sumElements.toFixed(2));


Why limit the result to only two decimal places?

Why limit the result to only two decimal places?

I used toFixed(2) earlier as a workaround for floating-point precision issues when the tests were using toBe, which checks for exact equality.
By rounding to two decimals, I could ensure the returned value matched the expected 0.44 style values.

Now that the tests use toBeCloseTo, the function no longer needs to round the result at all.
toBeCloseTo correctly handles floating-point precision, so I will remove the toFixed(2) rounding and return the raw sum.

Thank you for pointing this out, @cjyuan.

Iswanna · 2025-11-20T13:53:49Z

Tests are very comprehensive! I am impressed.

Thank you, @cjyuan.

- Remove redundant cloning of validNumbers before sorting - Allow safe in-place sort since validNumbers is newly created from list - Update comments to allign with changes

… handle non-numeric arrays

cjyuan

Changes look good. Well done!

cjyuan · 2025-11-20T16:21:08Z

Sprint-1/fix/median.js


-  // Create a shallow copy of `validNumbers` and sort it in ascending order
-  // This avoids modifying the original array
-  const sorted = [...validNumbers].sort((a, b) => a - b);


If you find sorted a more meaningful name for the array, you can also introduce it as

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

cjyuan · 2025-11-20T16:21:56Z

Sprint-1/implement/sum.js

    sumElements += item;
  }
-  return Number(sumElements.toFixed(2));
+  return Number(sumElements);


sumElements is already a number.

Iswanna · 2025-11-20T16:33:17Z

Changes look good. Well done!

Thank you so much @cjyuan for making the time to review my code and give detailed feedback. I really appreciate it.

Iswanna added 30 commits October 23, 2025 12:07

Add initial mean.js and mean.test.js files

6e09b64

Write a test for the function in the median.js file.

49b82d5

Implement calculateMedian function

94d5edf

Add functions, extra code, and explanatory comments to mean-median.js…

b6b2060

… about arrays as reference types

Add a return statement to the function calculateMean

935e47e

Export function to test file using module.exports

dfe0f44

Import calculateMean function from mean.js

22abc47

Import calculateMedian function from median.js

e0bed31

Add package.json file

e092070

Comment out function that mutates array with splice

4fe7d13

Add function to calculate median without mutating the array

f729643

This implementation handles arrays with an odd length.

Add Jest test case to verify that the function implementation does no…

3449f32

…t mutate the array

Add a Jest test case to check that calculateMedian returns the correc…

696f20b

…t median for an even-length array

Update function implementation to handle even-length arrays when calc…

2bf9d3c

…ulating the median

Remove function call and console.log debugging statements

f658cab

Fix inconsistent indentation in jest test

7ecf949

Add input validation to return null when a non-array is passed

c217567

Reformat median test suite for readability

d75a3ce

- Adjusted indentation and arrow function formatting - Added consistent line breaks and spacing - No changes to test logic or order

Add validation to ensure input is an array

4040d9b

Filter out non-numeric values from the array and assign its reference…

57c20f4

… to the `validNumbers`

Add statements to duplicate the input array and sort it in ascending …

8a41346

…order

Add logic to calculate the median for arrays with odd or even lengths

17d40f7

Add test case to ensure dedupe returns an empty array for empty input

a22309d

Make function accessible in other files by adding module.exports

8d14298

Add test case for dedupe to ensure it returns a copy of the array w…

50670f0

…hen input has no duplicates

Refactor test cases to data-driven format using an array and loop

63e538f

Add test case to ensure dedupe removes duplicates and keeps the first…

950a4a3

… occurrence for arrays of strings and numbers

Comment out unused test.todo

37d1133

Add additional test cases for no duplicates, duplicate and mixed stri…

1f17291

…ng-number array handling

Add case to verify dedupe removes invalid elements and duplicates, re…

a73b980

…turning a new array

Iswanna added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Groups The name of the module. 📅 Sprint 1 Assigned during Sprint 1 of this module labels Nov 10, 2025

cjyuan reviewed Nov 20, 2025

View reviewed changes

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

Iswanna added 3 commits November 20, 2025 12:53

Iswanna added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 20, 2025

cjyuan reviewed Nov 20, 2025

View reviewed changes

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

Iswanna added 4 commits November 20, 2025 14:17

Refactor calculateMedian by removing unnecessary array copy

68a0f81

- Remove redundant cloning of validNumbers before sorting - Allow safe in-place sort since validNumbers is newly created from list - Update comments to allign with changes

Fix errors and omissions in findMax tests: add missing assertions and…

3b01cee

… handle non-numeric arrays

Remove .toFixed(2) when returning the numeric value of sumElements

0a5cd81

Remove explicit precision from toBeCloseTo test cases

d112b5b

Iswanna added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 20, 2025

Iswanna requested a review from cjyuan November 20, 2025 15:22

cjyuan reviewed Nov 20, 2025

View reviewed changes

cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 20, 2025

Remove redundant Number() conversion around sumElement

b32105e

illicitonion closed this Jan 7, 2026

Uh oh!

West Midlands | 25 Sep ITP | Iswat Bello | Sprint 1 | Module Data Groups #829

West Midlands | 25 Sep ITP | Iswat Bello | Sprint 1 | Module Data Groups #829

Uh oh!

Conversation

Iswanna commented Nov 10, 2025

Learners, PR Template

Changelist

Questions

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Iswanna commented Nov 20, 2025

Uh oh!

Iswanna commented Nov 20, 2025

Uh oh!

cjyuan left a comment

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!

Iswanna commented Nov 20, 2025

Uh oh!

cjyuan left a comment

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!

Iswanna commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants