ITP_GLASGOW_APR | HANNA_MYKYTIUK | MODULE_DATA_GROUPS | SPRINT_1 #491
Conversation
cjyuan
added
the
Review in progress
| if (typeof list[i] == 'number'){ // add value to new array only if it is numeric | ||
| numbersList.push(list[i]); | ||
| } |
There was a problem hiding this comment.
Why check if the elements are of type "number" another time? (Isn't the code at lines 14-23 enough?)
| let middleIndex = Math.floor(numbersList.length / 2); | ||
| let median; | ||
| if ( numbersList.length%2 != 0 ){// check if array elements quantity is odd number | ||
| median = numbersList.splice(middleIndex, 1)[0]; |
There was a problem hiding this comment.
There is shorter syntax to access an array element.
There was a problem hiding this comment.
You missed specify two cases mentioned in this file:
// Given an empty array
// When passed to the dedupe function
// Then it should return an empty array
// Given an array with no duplicates
// When passed to the dedupe function
// Then it should return a copy of the original array
| // if elements does not contain numbers we should search for values that can be converted to numbers | ||
| if (containsNumbers == false){ | ||
| for (let i = 0; i < elements.length; i++){ | ||
| //check if variable can be converted to number and check if variable max is assigned | ||
| if (!(typeof max == 'number') && !isNaN(elements[i])){ | ||
| // if both true assign converted value to max | ||
| max = Number(elements[i]); | ||
| } | ||
| // if max is already assigned and elements[i] value can be converted to number | ||
| if (typeof max == 'number' && !isNaN(elements[i])){ | ||
| // check if new walue is greater than max | ||
| if (Number(elements[i]) > max){ | ||
| max = Number(elements[i]); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Most people choose to return -Infinity when all elements are non-number. That is, they treat such array as having no numbers.
If we initialised max to -Infinity, then we would not need to check !(typeof max == 'number') and we also do not need lines 19-21. Too much checking can degrade the performance of a program.
We can also return the value of max right before line 31. If the array does not contain any number, then we don't have to execute the remaining code.
| if (!(typeof max == 'number') && typeof elements[i] == 'number'){ | ||
| max = elements[i]; | ||
| } |
There was a problem hiding this comment.
We would not need these few lines if max was initialised to -Infinity.
| max = elements[i]; | ||
| } | ||
| // compare each numeric value with max, and replace max if new value greater | ||
| if (typeof max == 'number'){ |
There was a problem hiding this comment.
Shouldn't this check typeof elements[i] == 'number' instead?
| if (containsNumbers == false){ | ||
| for (let i=0; i<elements.length; i++){ | ||
| if(!isNaN(elements[i])){ | ||
| sum += Number(elements[i]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We could return the sum right before line 18 and avoid unnecessary processing.
| it("array with decimal/float numbers should return correct sum", () => { | ||
| const value = sum([7.1, 2.5, 0.6]); | ||
| expect(value).toEqual(10.2); | ||
| }); |
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(sum([1.2, 0.6, 0.005])).toEqual(1.805); // This fail
expect(sum([1.2, 0.6, 0.005])).toEqual(1.8049999999999997); // This pass
expect(sum([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); // false
Can you find a more appropriate way to test a value (that involves decimal number calculations) for equality?
cjyuan
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.