Skip to content

Sheffield | 26-Jan-ITP | Daniel Aderibigbe | Sprint 1 | Coursework/sprint 1#986

Open
Dan2Clouted wants to merge 10 commits intoCodeYourFuture:mainfrom
Dan2Clouted:coursework/sprint-1
Open

Sheffield | 26-Jan-ITP | Daniel Aderibigbe | Sprint 1 | Coursework/sprint 1#986
Dan2Clouted wants to merge 10 commits intoCodeYourFuture:mainfrom
Dan2Clouted:coursework/sprint-1

Conversation

@Dan2Clouted
Copy link
Copy Markdown

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

Completed all exercises.

@Dan2Clouted Dan2Clouted added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 9, 2026
Comment thread Sprint-1/implement/dedupe.js Outdated
Comment thread Sprint-1/implement/dedupe.test.js
Comment thread Sprint-1/implement/max.test.js Outdated
Comment thread Sprint-1/implement/max.js
Comment thread Sprint-1/implement/sum.js
Comment thread Sprint-1/implement/sum.test.js
Comment thread Sprint-1/implement/sum.test.js Outdated
Comment on lines +70 to +83
test("given an array containing non-number values, it ignores them and returns the sum of the numerical elements", () => {
const input = ["hello", "world"];
const expectedOutput = 0;
expect(sum(input)).toBe(expectedOutput);
});

// Given an array with only non-number values
// When passed to the sum function
// 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, it returns the least surprising value", () => {
const input = ["hello", "world"];
const expectedOutput = 0;
expect(sum(input)).toBe(expectedOutput);
});
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.

Duplicate

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.

I think this exercise also involves "reading data from files". If you are up to the challenge, can you write a script that can read the numbers from a file and then output their sums?

@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 Mar 17, 2026
@Dan2Clouted
Copy link
Copy Markdown
Author

Thanks for the detailed feedback — really helpful.

I’ve gone through all the comments and made the following updates:

  • Refactored loops (e.g. using for...of where appropriate and adding braces for clarity)
  • Improved tests to ensure correctness, including checking that returned arrays are new copies rather than the original reference
  • Handled edge cases in functions like max and sum, including ignoring NaN and non-finite values
  • Updated decimal tests to use toBeCloseTo to avoid floating point precision issues
  • Removed duplicate tests and improved test coverage where needed

@Dan2Clouted Dan2Clouted added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 19, 2026
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Changes look good, but you introduced a bug into one of the files. Can you fix it?

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 19, 2026
@Dan2Clouted
Copy link
Copy Markdown
Author

The bug issues has been resolved.

@Dan2Clouted Dan2Clouted added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 19, 2026
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Mar 19, 2026

I can still see the bug. If you run jest test, you should notice some failed tests.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 19, 2026
@Dan2Clouted
Copy link
Copy Markdown
Author

I think now it should have been fixed.

@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Mar 20, 2026

No new commits pushed to GitHub, and label not yet changed.

@Dan2Clouted
Copy link
Copy Markdown
Author

Sorry for all this, But i think it should be fixed now. Thank you for your time.

@Dan2Clouted Dan2Clouted added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 20, 2026
@cjyuan
Copy link
Copy Markdown
Contributor

cjyuan commented Mar 20, 2026

One of the commit fixed the bug but it wasn't pushed to GitHub before. However, a newer commit reintroduced the bug.

So the bug remains.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 20, 2026
@Dan2Clouted
Copy link
Copy Markdown
Author

It should be fixed now. Thank you for your time and patience.

@cjyuan cjyuan removed the Reviewed Volunteer to add when completing a review with trainee action still to take. label Mar 21, 2026
@cjyuan cjyuan added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Mar 21, 2026
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants