Repository navigation
Manchester | 26-ITP-Sep | Salah Alsabhi | Sprint 1 | Coursework - #1482
salahalsabhi wants to merge 8 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
salahalsabhi
changed the title
This comment has been minimized.
This comment has been minimized.
salahalsabhi
changed the title
This comment has been minimized.
This comment has been minimized.
salahalsabhi
changed the title
salahalsabhi
added
the
Needs Review
|
Ready for review. Completed Sprint 1:
All tests passing locally with |
| // Given an array with no duplicates | ||
| // When passed to the dedupe function | ||
| // Then it should return a copy of the original array | ||
| test("Given an array with no duplicates, should return a copy of the original array", () => { | ||
| expect([5, 1, 2, 3, 8]).toEqual([5, 1, 2, 3, 8]); | ||
| }); |
There was a problem hiding this comment.
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 25:
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?
There was a problem hiding this comment.
How should the function handle an array containing NaN?
There was a problem hiding this comment.
What should calculateMean([Infinity, 0, -Infinity]) or calculateMean([0, NaN]) return?
| test("Given an array containing decimal numbers, should return the correct mean", () => { | ||
| expect(calculateMean([2.2, 1.9, 3.5, 4.1])).toEqual(2.925); | ||
| }); | ||
|
|
||
| test("Given an array containing both negative and decimal numbers, should return the correct mean", () => { | ||
| expect(calculateMean([2.2, -1.9, 3.5, -4.1])).toEqual(-0.07499999999999984); | ||
| }); |
There was a problem hiding this comment.
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); // falseCan 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
| // Puzzle input | ||
| const changes = fs | ||
| .readFileSync(new URL("./input.txt", import.meta.url), "utf8") | ||
| .trim() | ||
| .split("\n") | ||
| .map(Number); |
There was a problem hiding this comment.
Could you explain the purpose of calling trim() on the input before applying split('\n')?
| for (let change of changes) { | ||
| frequency += change; | ||
| } |
There was a problem hiding this comment.
Why not declare the loop variable change using const?
cjyuan
added
Reviewed
Learners, PR Template
Self checklist
Task code
CYF-1071
Changelist
Sprint 1: fix, implement, refactor, stretch
Questions
No questions