London | 26-ITP-May | Damilola Odumosu | Sprint 1 | Coursework - #1296
London | 26-ITP-May | Damilola Odumosu | Sprint 1 | Coursework#1296d-odumosu wants to merge 6 commits into
Conversation
… of loop. all tests pass.
d-odumosu
added
Needs Review
cjyuan
added
Review in progress
| const arrCopy = [...list]; | ||
| const filteredNumbers = arrCopy.filter((num) => Number.isFinite(num)); |
There was a problem hiding this comment.
.filter() returns a new array without mutating the original array, there is no need to clone another array.
| if (elements.length === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| return elements.filter((item, index) => elements.indexOf(item) === index); |
There was a problem hiding this comment.
Could we expect line 6 to work also when array length is 0?
| test("given an array with no duplicates, return a copy of the array", () => { | ||
| expect(dedupe(input)).toEqual(expected); | ||
| }); |
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?
| if (elementLists.length === 0) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
When a function is expected to return a number, it is better to design the function to always returned a value of type number for consistency.
Technically, an empty array also an array containing no numbers.
| const testCaseArrayWithOnlyNonNumberValues = [ | ||
| { input: ["hello", "world"], expected: undefined }, | ||
| { input: [null, undefined, true, false], expected: undefined }, | ||
| { input: [{}, [], "test"], expected: undefined }, | ||
| ]; |
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.
| const elementsList = elements.filter((element) => { | ||
| return typeof element === "number"; | ||
| }); |
There was a problem hiding this comment.
What do you expect from the following function calls (on extreme cases)?
Does your function return the value you expected?
sum([NaN, 1]);
sum([Infinity, -Infinity]);
| let sum = 0; | ||
| for (let i = 0; i < elementsList.length; i++) { | ||
| sum += elementsList[i]; | ||
| } | ||
| return sum; |
There was a problem hiding this comment.
Could this code work for an array of any length?
| for (let index = 0; index < list.length; index++) { | ||
| const element = list[index]; |
There was a problem hiding this comment.
Your code works.
However, to find if an element exists in an array, the function could stop searching the array as soon as it finds a match; it doesn't necessary need to always search the whole array.
Could you improve your implementation? (You could also just modify two lines of the original code.)
cjyuan
added
Reviewed
2 participants
Self checklist
Changelist
Sprint 1 Tasks
fix :
implement:
dedupe
max
sum
refactor: