Skip to content

Manchester |26-ITP-Sep | Precious Moses | Sprint 1 | Data Groups - #1479

Open
moses77-boop wants to merge 60 commits into
CodeYourFuture:mainfrom
moses77-boop:Sprint-1
Open

moses77-boop wants to merge 60 commits into
CodeYourFuture:mainfrom
moses77-boop:Sprint-1

Conversation

@moses77-boop

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

Task code

CYF-1071

Changelist

In this PR, I refactored codes, implemented functions and test codes to complete the coursework.

@moses77-boop moses77-boop 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. labels Oct 23, 2026
Comment on lines +58 to +72
test("given a value that is not an array, throws an error", () => {
expect(() => calculateMean("banana")).toThrow("calculateMean requires an array of numbers");
});

test("given a value that is not an array, throws an error", () => {
expect(() => calculateMean(42)).toThrow("calculateMean requires an array of numbers");
});

test("given a value that is not an array, throws an error", () => {
expect(() => calculateMean(null)).toThrow("calculateMean requires an array of numbers");
});

test("given a value that is not an array, throws an error", () => {
expect(() => calculateMean({})).toThrow("calculateMean requires an array of numbers");
});

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 consider grouping these test cases into one category; they have the same test description.

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.

Code refactored.

@@ -0,0 +1,19 @@
const fs = require("fs");
const input = fs.readFileSync("input.txt", "utf8");
const changes = input.trim().split("\n").map(Number);

@cjyuan cjyuan Oct 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.

Could you explain why you choose to apply .trim() to the input before applying split()?

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.

.trim() removes the trailing newline so I don't get an empty last entry.

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.

Note: For large file, calling trim() just to remove the trailing newline is not efficient. Do you know why?

No change needed.

Comment on lines +8 to +17
while (true) {
for (const change of changes) {
frequency += change;
if (seen.has(frequency)) {
console.log(frequency);
process.exit(0);
}
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.

Is this implementation for the second part of the problem? I think the first part is just adding all the frequencies.

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 actually wrote the part 1 first and refactored the same code for part 2.
Do I need to write both parts of the code separately?

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.

No need. Knowing how to read lines of data from a file is more important in this exercise.

Comment thread Sprint-1/implement/sum.js Outdated
let total = 0;

for (const element of elements) {
if (typeof element !== "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.

Note: NaN and positive/negative Infinity are also value of type numbers. If it is up to you, how would you design the function when the given array contain such values?

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 have refactored the code.

if (list.length === 0) {
throw new Error("calculateMean requires a non-empty 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.

Spacing a bit inconsistent after if.

@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

Changes look good. Well done.

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

cjyuan commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Note: You should also change the label to "Needs review" after you have made all the changes.

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. Module-Data-Groups The name of the module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants