Skip to content

Manchester | 26-ITP-Sep | Mathew Shaibu | Sprint 1 | Module data groups sprint-1 - #1480

Open
MattCarolus10 wants to merge 63 commits into
CodeYourFuture:mainfrom
MattCarolus10:Module-Data-Groups-sprint-1
Open

MattCarolus10 wants to merge 63 commits into
CodeYourFuture:mainfrom
MattCarolus10:Module-Data-Groups-sprint-1

Conversation

@MattCarolus10

Copy link
Copy Markdown

No description provided.

@MattCarolus10 MattCarolus10 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 27, 2026
Comment on lines -21 to -23
// Given an array with no duplicates
// When passed to the dedupe function
// Then it should return a copy of the original array

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.

  • You missed implementing this test.

  • Please note that in this test, in addition to checking the returned array has the same elements, you should also test whether the returned array is a different array.

Comment on lines -25 to +13
// Given an array of strings or numbers
// When passed to the dedupe function
// Then it should return a new array with duplicates removed while preserving the
// first occurrence of each element from the original array.
test("keeps the first occurrence order", () => {
expect(dedupe([5, 1, 1, 2, 3, 2, 5, 8])).toEqual([5, 1, 2, 3, 8]);
});

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.

  • The comment on line 25 suggests testing an array containing mixed types of values.

  • We could test multiple test cases in one test, and doing so gives us a better chance to catch bugs.

  • I would also suggest using AI to research what would make good test cases and, more importantly, why. There may be valuable reasoning and insights that we can learn from AI. In fact, understanding the "why" is more important than simply identifying the "what". (This applies to all tests in this exercise)

Comment thread Sprint-1/implement/sum.js
if (!Array.isArray(list)) {
throw new Error("sum requires an array of numbers");
}
if (!list.every((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.

+/- Infinity and NaN are also of type "number". If it were up to you, how would you design the function to handle these values?

@@ -0,0 +1,16 @@
const fs = require("fs");
const changes = fs.readFileSync("input.txt", "utf8").trim().split("\n").map(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.

Could you walk me through why trim() is needed before applying split("\n") on the input?

Comment on lines +11 to +15
if (seen.has(frequency)) {
console.log(frequency);
break;
}
seen.add(frequency)

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.

Indentation is off.

@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 Oct 31, 2026
@cjyuan

cjyuan commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Could you prepare and format the PR description following the same structure and style as the example in this PR?
Please use it as a reference for the layout, headings, and level of detail.

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