Glasgow| 2026-ITP-Jan | Tuan Nguyen| Sprint 1 | Array-and-Loop. by Jacknguyen4438 · Pull Request #1189 · CodeYourFuture/Module-Data-Groups

Jacknguyen4438 · 2026-04-09T22:45:13Z

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

Hello I make this PR ready for review. I have learn and practice on how to combine loop and array.

Questions

No question.

cjyuan

Can you remove "Alarm Clock" from the PR title?

cjyuan · 2026-04-10T00:32:19Z

Sprint-1/fix/median.test.js

+    { input: [1, 2, 3, 4], expected: 3 },
+    { input: [1, 2, 3, 4, 5, 6], expected: 4 },


You were supposed to fix the function in median.js.

There is nothing wrong with these original test data.

{ input: [1, 2, 3, 4], expected: 2.5 }, { input: [1, 2, 3, 4, 5, 6], expected: 3.5 },

Can you restore the original tests? You shouldn't need to modify median.test.js.

Thank you for the feed back, I have restored the test to the original state.

cjyuan · 2026-04-10T00:35:06Z

Sprint-1/implement/dedupe.test.js

 // Given an array with no duplicates
 // When passed to the dedupe function
 // Then it should return a copy of the original array
-
+test.todo("given an array, it returns should return to original array");
+test("with input of empty array should return, empty array", () => {
+  let normalArray = [1, 2, 3, 4, 5, 6, 10];
+  expect(dedupe(normalArray)).toEqual(normalArray);
+});


This test should fail if the function returns the original array (instead of a copy of the original array).

The current test checks only if both the original array and the returned array contain identical elements.
In order to validate the returned array is a different array, we need an additional check.

Can you find out what this additional check is?

Hello thank you for the feed back, I could see why you point this out. I have check with the function and see that error pop up since. If I pass the normalArray and make any change that when return a new normalArray that do not have the same as the content if will result in error. So here my code for additional check:

test("with input of array with no duplicates should return a copy of original array", () => {
let normalArray = [1, 2, 3, 4, 5, 6, 10];
expect(dedupe(normalArray)).toEqual(normalArray);
expect(dedupe(normalArray)).not.toBe(normalArray);
});

cjyuan · 2026-04-10T00:35:57Z

Sprint-1/implement/max.js

+  let filteredArray = elements.filter(
+    (item) => typeof item === "number" && !isNaN(item)
+  );
+  if (filteredArray.length === 0) return null;
+  let sortedArray = filteredArray.sort((a, b) => a - b);
+  let maxArrayValue = sortedArray[sortedArray.length - 1];


If a variable is not going to be reassigned a value, best practice is to declare them using const instead of let.

Thank you for the advice. I intentional use let because it can help the variable inside the function changing the value without issue. So I will try and replace all the let to const if the variable will not be reassign the value later.

cjyuan · 2026-04-10T00:38:17Z

Sprint-1/implement/max.test.js

 test.todo("given an empty array, returns -Infinity");
+//Hello I would  like to keep this to help check the list of test need to be done.
+test("when an empty array push into the function it should return empty array", () => {
+  let emptyArray = [];
+  expect(findMax(emptyArray)).toEqual([]);
+});


The requirement is to return -Infinity when the array is empty.

cjyuan · 2026-04-10T00:39:42Z

Sprint-1/implement/max.test.js

+test("When an array have decimal number value", () => {
+  let decimalArray = ["Hello", "Hi", 1, 12312321n, undefined];
+  expect(findMax(decimalArray)).toBe(1);
+});


The test description does not quite describe the test.

Thank you for the advice here is my change base on the your advice:

test("When an array have number value mix with non-number value, the return should only be number value", () => {
let decimalArray = ["Hello", "Hi", 1, 12312321n, undefined];
expect(findMax(decimalArray)).toBe(1);
});

cjyuan · 2026-04-10T00:40:21Z

Sprint-1/implement/max.test.js

+test.todo("given an array with decimal number, returns the largest decimal");
+test("When an array have decimal number value", () => {
+  let decimalArray = [2.3, 1.5, 6.5, 10.2, 11.5];
+  expect(findMax(decimalArray)).toBe(11.5);
+});


The test.todo() is a place holder. When you have implemented the corresponding tests, you can remove them.

Hello and thank you for the review advice. I was intent to left is the test to help me keep track and see how many test I need to do so now I will remove all of them.

cjyuan · 2026-04-10T00:42:43Z

Sprint-1/implement/sum.test.js

+test("given an array with 1 number value, returns that number value", () => {
+  let mixedArray2 = ["Hello", 20.17, null, 9.7, undefined];
+  expect(sum(mixedArray2)).toBe(29.87);
+});


Decimal numbers in most programming languages (including JS) are internally represented in "floating point number" format. Floating point arithmetic is not exact. For example, the result of 46.5678 - 46 === 0.5678 is false because 46.5678 - 46 only yield a value that is very close to 0.5678. Even changing the order in which the program add/subtract numbers can yield different values.

So the following could happen

expect( 1.2 + 0.6 + 0.005 ).toEqual( 1.805 ); // This fail expect( 1.2 + 0.6 + 0.005 ).toEqual( 1.8049999999999997 ); // This pass expect( 0.005 + 0.6 + 1.2 ).toEqual( 1.8049999999999997 ); // This fail console.log(1.2 + 0.6 + 0.005 == 1.805); // false console.log(1.2 + 0.6 + 0.005 == 0.005 + 0.6 + 1.2); // false

Can you find a more appropriate way to test a value (that involves decimal number calculations) for equality?

Suggestion: Look up

Checking equality in floating point arithmetic in JavaScript

Checking equality in floating point arithmetic with Jest

cjyuan · 2026-04-10T00:43:20Z

Sprint-1/implement/sum.test.js

 // When passed to the sum function
 // Then it should still return the correct total sum
-
+test.todo("given an array with negative number, returns correct total sum");


You should also remove all test.todo() in this file after you have implemented the corresponding tests.

Thank you for the advice I have remove all the test.todo you can comfirm this when I asak for re-review PR.

Jacknguyen4438 · 2026-04-12T00:28:15Z

@cjyuan Hello and Thank you for reading this PR I have finish fixing the course and it ready to be review again

Jacknguyen4438 added 9 commits April 6, 2026 14:44

Making some fix for the jest test

ff7f2fc

fix the jest to correction and finish first part

7fb1d60

Make some basic change for the task

558b61f

make a list of to do jest test for max.js

d17be19

making some jest test for course work

a647b10

Make some final jest test for submission

530533c

make some change for max.js jest test

adf5466

making change and jest test for sum

bbd2d7e

Format sum.js,max.js,dedupe.js and add test to dedupe.test.js

591925a

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

cjyuan reviewed Apr 10, 2026

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 Apr 10, 2026

Jacknguyen4438 added 3 commits April 11, 2026 00:22

Remove all todo each jest test

bf6c830

Basic change

1f124e8

making some change for PR

1665dd5

Jacknguyen4438 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 12, 2026

		{ input: [1, 2, 3, 4], expected: 3 },
		{ input: [1, 2, 3, 4, 5, 6], expected: 4 },

Uh oh!

Conversation

Jacknguyen4438 commented Apr 9, 2026

Learners, PR Template

Changelist

Questions

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!

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!

Jacknguyen4438 commented Apr 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants