Glasgow | Jan-26-ITP | Alasdair MacDonald | Sprint 1 | Exercises#997
Glasgow | Jan-26-ITP | Alasdair MacDonald | Sprint 1 | Exercises#997MacDonald91 wants to merge 1 commit intoCodeYourFuture:mainfrom
Conversation
MacDonald91
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
You missed updating one file:
Sprint-1/refactor/includes.js
| const median = list.splice(middleIndex, 1)[0]; | ||
| return median; | ||
| // keep only numeric values | ||
| const numbers = list.filter(value => typeof value === "number"); |
There was a problem hiding this comment.
Do you plan to consider also -Infinity, Infinity, and NaN in the median calculation (and in the functions in implement/max.js and implement/sum.js)?
| // sort numbers without modifying original list | ||
| const sorted = [...numbers].sort((a, b) => a - b); |
There was a problem hiding this comment.
Is numbers the original array?
| // 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, it returns the same array", () => { | ||
| expect(dedupe([1, 2, 3])).toEqual([1, 2, 3]); | ||
| }); |
There was a problem hiding this comment.
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?
| test("ignores non-number values", () => { | ||
| expect(findMax(["hey", 10, "hi", 60, 10])).toBe(60); | ||
| }); | ||
|
|
||
| // Given an array with only non-number values | ||
| // When passed to the max function | ||
| // Then it should return the least surprising value given how it behaves for all other inputs | ||
| // Then it should return the least surprising value | ||
| test("only non-number values returns -Infinity", () => { | ||
| expect(findMax(["a", "b", "c"])).toBe(-Infinity); | ||
| }); No newline at end of file |
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.
Why return a string (instead of a value of type "number") when the given array contains only non-number values?
When a function has a dual return type, it becomes unclear what the caller should expect. Developers would need to look at the implementation or documentation to understand the behavior.
| test("given decimal numbers", () => { | ||
| expect(sum([1.5, 2.5, 3])).toBe(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); // 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
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Completed Sprint 1 exercises.
Questions
n/a