Repository navigation
London | 26-ITP-Sep | Mahir Shah | Sprint 1 | Data Groups - #1481
MahirShah300 wants to merge 14 commits into
Conversation
MahirShah300
added
the
Needs Review
cjyuan
added
Review in progress
| !list.every( | ||
| (currentValue) => | ||
| typeof currentValue === "number" && !Number.isNaN(currentValue) | ||
| ) |
There was a problem hiding this comment.
-
Could also consider excluding +/-
Infinity. -
Could probably give a simpler name than
currentValueto the parameter. The prefix "current" does not add any new info.
| for (value of elements) { | ||
| if (!dedupedArray.includes(value)) { | ||
| dedupedArray.push(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should the loop variable value be declared using const or let in this loop?
| // 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 same array", () => { | ||
| expect(dedupe([1, 5, 9, 23])).toEqual([1, 5, 9, 23]); | ||
| }); |
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 specified in the original comment:
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 (list.length === 0) { | ||
| return "Could not calculate a median: calculateMedian requires a non-empty array"; | ||
| } | ||
|
|
||
| if (!Array.isArray(list)) { | ||
| return "Could not calculate a median: calculateMedian requires an array of numbers"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you look up how to use a try...catch block to catch a thrown error object, then simplify the code on lines 27-33 into a single line by constructing the return value from the error object's message property?
| sortedElements = elements.toSorted((a, b) => a - b); | ||
|
|
||
| return sortedElements[sortedElements.length - 1]; |
There was a problem hiding this comment.
This work.
Suggestion: Explore a more efficient approach to find the largest numbers in an array.
| !list.every( | ||
| (currentValue) => | ||
| typeof currentValue === "number" && !Number.isNaN(currentValue) | ||
| ) |
There was a problem hiding this comment.
-
Infinityis also a value of type "number". -
Could also consider importing
sum()fromsum.jsand use it in this script.
| test("given an array with decimal numbers, correctly return total sum", () => { | ||
| expect(sum([12.7, 3.6, 32.48, 8, 32.53])).toEqual(89.31); | ||
| }); |
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.
console.log(12.7 + 3.6 + 32.48 + 8 + 32.53 == 89.31); // true
// When numbers are added in different order
console.log(32.53 + 8 + 32.48 + 3.6 + 12.7 == 89.31); // 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
| function includes(list, target) { | ||
| for (let index = 0; index < list.length; index++) { | ||
| const element = list[index]; | ||
| for (element of list) { |
There was a problem hiding this comment.
Loop variable element has never been declared.
cjyuan
added
Reviewed
Learners, PR Template
Self checklist
Task code
CYF-1071
Changelist
Implemented tests and functions.