Skip to content

Birmingham | 26-ITP-May | Toluwalase Tiamiyu | Sprint 1 | Exercises - #1295

Open
TTiamiyu wants to merge 13 commits into
CodeYourFuture:mainfrom
TTiamiyu:data-groups-sprint1
Open

Birmingham | 26-ITP-May | Toluwalase Tiamiyu | Sprint 1 | Exercises#1295
TTiamiyu wants to merge 13 commits into
CodeYourFuture:mainfrom
TTiamiyu:data-groups-sprint1

Conversation

@TTiamiyu

@TTiamiyu TTiamiyu commented Jul 26, 2026

Copy link
Copy Markdown

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

@github-actions

This comment has been minimized.

5 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@TTiamiyu TTiamiyu changed the title Birmingham | ITP MAY 2026 | Toluwalase Tiamiyu | Sprint 1 | Data Group Coursework Birmingham | ITP MAY 2026 | Toluwalase Tiamiyu | Sprint 1 | Coursework Jul 26, 2026
@github-actions

This comment has been minimized.

@TTiamiyu TTiamiyu changed the title Birmingham | ITP MAY 2026 | Toluwalase Tiamiyu | Sprint 1 | Coursework Birmingham | ITP MAY 2026 | Toluwalase Tiamiyu | Sprint 1 | Data groups Jul 26, 2026
@github-actions

This comment has been minimized.

@TTiamiyu TTiamiyu changed the title Birmingham | ITP MAY 2026 | Toluwalase Tiamiyu | Sprint 1 | Data groups Birmingham | 26-ITP-May | Toluwalase Tiamiyu | Sprint 1 | Execises Jul 26, 2026
@github-actions

This comment has been minimized.

@TTiamiyu TTiamiyu added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 26, 2026
@github-actions

This comment has been minimized.

@github-actions github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 26, 2026
@github-actions

This comment has been minimized.

@TTiamiyu TTiamiyu changed the title Birmingham | 26-ITP-May | Toluwalase Tiamiyu | Sprint 1 | Execises Birmingham | 26-ITP-May | Toluwalase Tiamiyu | Sprint 1 | Exercises Jul 26, 2026
@github-actions

This comment has been minimized.

@TTiamiyu TTiamiyu added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 26, 2026
@cjyuan cjyuan added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jul 31, 2026
Comment thread Sprint-1/implement/dedupe.test.js Outdated
Comment on lines +23 to +25
test("given an array with no duplicates, it returns a copy of the original array", () => {
expect(dedupe([1, 2, 3])).toEqual([1, 2, 3]);
});

@cjyuan cjyuan Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Your function implementation is correct. However, this test could be improved to better ensure
that any future changes continue to align with the expected behavior described on line 29:

Then it should return a copy of the original array

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

i found that using a .toBe and .not.toBe assertions would work better than .toEqual assertion for this test, i have changed the assertions and also added extra assertions to cater for future changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

toEqual() and toBe() serve differerent purposes. In this test you need both.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

that is very true, most of the test cases in the .test.js file have both assertions to check the correctness of the values and also protect against just returning the list as is.

Comment on lines +62 to +64
test("given an array with only non-number values, return the least surprising value", () => {
expect(findMax(["a", "c", "e"])).toBe(-Infinity);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When a string representing a valid numeric literal (for example, "300") is compared to a number,
JavaScript first converts the string into its numeric equivalent before performing the comparison.
As a result, the expression 20 < "300" evaluates to true.

To test if the function can correctly ignore non-numeric values, you may want to include a string such as "300" in the relevant test cases.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thank you for pointing that out, i did not think of that and i have adjusted the test case to include a string with numbers.

Comment thread Sprint-1/implement/sum.js Outdated
@@ -1,4 +1,6 @@
function sum(elements) {
const numbers = elements.filter((item) => typeof item === "number");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you expect from the following function calls (on extreme cases)?
Does your function return the value you expected?

sum([NaN, 1]);
sum([Infinity, -Infinity]);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No, it wouldn’t return what is expect, i have Incorporated these extreme cases in the test and also improved the sum.js code to cater for such extremes.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants