Manchester | 26-ITP-May | Yee Man Tsang | Sprint 1 | Data Group Coursework - #1359
Manchester | 26-ITP-May | Yee Man Tsang | Sprint 1 | Data Group Coursework#1359lintsang wants to merge 2 commits into
Conversation
lintsang
added
the
Needs Review
cjyuan
added
Review in progress
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Tests are quite comprehensive.!
| const listNumberOnlyAsc = [...list].filter( | ||
| (item) => !isNaN(item) && item !== null | ||
| ); |
There was a problem hiding this comment.
- Why clone the original array when using
.filter()? (This comment applies to all the code that that uses.filter().)
| const listNumberOnlyAsc = [...list].filter( | ||
| (item) => !isNaN(item) && item !== null | ||
| ); | ||
| if (listNumberOnlyAsc == undefined || listNumberOnlyAsc == 0) { |
There was a problem hiding this comment.
Will the condition on line 14 ever be true?
| const listPureNumber = [...listNumberOnlyAsc].filter( | ||
| (item) => typeof item == "number" | ||
| ); |
There was a problem hiding this comment.
-
Why not include this filtering condition in the filter on lines 11-13 so that we only need to filter the array once?
-
You may also want to check out the methods in Number to see if there is any suitable method you can use to retain valid numbers.
|
|
||
| if (listSorted.length % 2 === 1) { | ||
| const middleIndex = Math.floor(listSorted.length / 2); | ||
| const median = [...listSorted].splice(middleIndex, 1)[0]; //code '.splice(middleIndex,1)' means remove the middle index num in array and put it in a new array, '[0]' is the first item in this new array |
There was a problem hiding this comment.
Why not use the same syntax you are using on line 30?
| } else { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
We could also express
if (condition) {
... // Code to handle normal cases
} else {
return null;
}
as
if (!condition) { // Opposite condition
return null;
}
... // Code to handle normal cases
This way, we don't need to use else. This is possible only if the else part has a return statement.
| const elementsSorted = elementsPureNumber.sort((a, b) => a - b); | ||
| const elementsMax = elementsSorted.toSpliced(0, elementsSorted.length - 1); | ||
| return elementsMax[0]; |
There was a problem hiding this comment.
-
You could also just return the last element of the array without first deleting elements from the array.
-
You could also sort the array in reverse order and access its first element.
| const elementsNumberOnly = [...elements].filter((element) => !isNaN(element)); | ||
| const elementsPureNumber = [...elementsNumberOnly].filter( | ||
| (item) => typeof item == "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]);
| } else if (elementsPureNumber.length > 1) { | ||
| const iterator = elementsPureNumber.values(); | ||
| let sumOfArray = 0; | ||
| for (const value of iterator) { |
There was a problem hiding this comment.
for-of loop works on array directly (arrays are iterable). You could just write for (const value of elementsPureNumber) { .
| if (elementsPureNumber.length === 0) { | ||
| return 0; | ||
| } else if (elementsPureNumber.length === 1) { | ||
| return elementsPureNumber[0]; | ||
| } else if (elementsPureNumber.length > 1) { | ||
| const iterator = elementsPureNumber.values(); | ||
| let sumOfArray = 0; | ||
| for (const value of iterator) { | ||
| sumOfArray += value; | ||
| } | ||
| return sumOfArray; | ||
| } |
There was a problem hiding this comment.
The code on lines 13-17 also works for array of any size. If you prefer having less code, you could consider using the same code to handle all cases. That way, you don't need to use if-else.
| { input: [1.1, 1.2], expected: 2.3 }, | ||
| { input: [-1.1, -2.4, 1.1], expected: -2.4 }, | ||
| { input: [-15.38, -3.2, -5.01, -10.0], expected: -33.59 }, | ||
| ].forEach(({ input, expected }) => | ||
| it(`returns the sum in array contains decimal number [${input}]`, () => | ||
| expect(sum(input)).toEqual(expected)) | ||
| ); |
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
Finished all exercises in Sprint 1 folder