Skip to content

Conversation

@mohsenzamanist
Copy link

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 tasks of Sprint 1: fix, implement, refactor, stretch folders

Questions

  1. While creating the tests, deleted some of the comments explaining what each test should do, because the description on each test was self-explanatory. Is it a good practice or the original comments must stay?
  2. What is the best practice to add comments? When do comments must be added?

@mohsenzamanist mohsenzamanist added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Feb 2, 2026
},
].forEach(({ input, expected }) =>
it("returns a copy of original array when passed array with no duplicates ", () =>
expect(dedupe(input)).toEqual(expected))
Copy link
Contributor

Choose a reason for hiding this comment

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

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 implement this additional check?

Comment on lines 51 to 52
it("returns the correct sum for array containing decimal/float numbers", () =>
expect(sum(input)).toBe(expected))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@mohsenzamanist mohsenzamanist Feb 12, 2026

Choose a reason for hiding this comment

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

That was interesting. Thank you.
And hope I got it right.

@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 Feb 12, 2026
@cjyuan
Copy link
Contributor

cjyuan commented Feb 12, 2026

While creating the tests, deleted some of the comments explaining what each test should do, because the description on each test was self-explanatory. Is it a good practice or the original comments must stay?

You can delete the original comments if the same information can be found in the test description.

What is the best practice to add comments? When do comments must be added?

  • Don't state the obvious (e.g., if a variable name is self explanatory, then we don't have to repeat the info in a comment)
  • Comments should aide others (or yourself in the future) understand the code, or why you did things in a certain way.
  • Keep the comments concise

I think ChatGPT can probably give you a more complete suggestion.

@mohsenzamanist mohsenzamanist added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Feb 12, 2026
Copy link
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.

Just the part that checks "if the function returns a copy of the original array" is not quite correct. Can you fix it?

it(
"returns a copy of original array when passed array with no duplicates ",
() => expect(dedupe(input)).toEqual(expected),
expect(dedupe(input)).not.toBe(expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 33 does not quite check if the function returns a copy of the original array.

{ input: [-7958463, -100, -202, -6453], expected: -7965218 },
].forEach(({ input, expected }) =>
it("returns the correct sum for array with only negative values", () =>
expect(sum(input)).toBeCloseTo(expected))
Copy link
Contributor

Choose a reason for hiding this comment

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

If the input does not involve decimal numbers, toEqual() is better.

All integers in the interval [-Number.MAX_SAFE_INTEGER, Number.MAX_SAFE_INTEGER] can be represented precisely.

@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 Feb 13, 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