Cape Town | 26-ITP-Jan | Pretty Taruvinga | Sprint 1 | Data Groups#1104
Cape Town | 26-ITP-Jan | Pretty Taruvinga | Sprint 1 | Data Groups#1104Pretty548 wants to merge 11 commits intoCodeYourFuture:mainfrom
Conversation
…om>fix: correct calculateMedian implementation and handle non-array inputs
This comment has been minimized.
This comment has been minimized.
Pretty548
added
the
Needs Review
| // 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, when passed to the dedupe function, then it should return a copy of the original array", () => { | ||
| expect(dedupe(["a", "b", "c"])).toEqual(["a", "b", "c"]); | ||
| }); |
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 34:
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?
| } | ||
| if (max === -Infinity) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The spec in max.test.js is:
// Given an empty array
// When passed to the max function
// Then it should return -Infinity
| test("Given an array with non-number values, when passed to the max function, then it should return the max and ignore non-numeric values", () => { | ||
| expect(findMax(["hey", 10, "hi", 60, 10])).toBe(60); | ||
| }); |
There was a problem hiding this comment.
When a string representing a valid numeric literal (for example, "300") is compared to a number,
JavaScript first converts the string into its numeric equivalent before performing the comparison.
As a result, the expression 20 < "300" evaluates to true.
To test if the function can correctly ignore non-numeric values,
consider including a string such as "300" in the relevant test cases.
| total += elements[i]; | ||
| } | ||
| } | ||
| return Number(total.toFixed(10)); |
| test("Given an array with decimal/float numbers, when passed to the sum function, then it should return the correct total sum", () => { | ||
| expect(sum([1.5, 2.3, 0.8, 3.1])).toBe(7.7); | ||
| }); |
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); // falseInstead of reducing the precision of the calculation in the sum() function, 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
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist