Manchester | 26-ITP-Jan | Ofonime Edak | Sprint 1 | Data group#1045
Manchester | 26-ITP-Jan | Ofonime Edak | Sprint 1 | Data group#1045Ofonimeedak wants to merge 5 commits intoCodeYourFuture:mainfrom
Conversation
Ofonimeedak
added
📅 Sprint 1
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Why not practice "committing files one by one, on purpose, and for a reason"?
In VSCode, you can select which file to stage, and commit only the staged file.
See: https://www.youtube.com/watch?v=z5jZ9lrSpqk&t=705 (At around 12:50 minute marker, the video shows how to stage a single file).
| [{ input: [0.5, 0.2, 0.11, 0.89, 0.3], expected: 2 }].forEach( | ||
| ({ input, expected }) => | ||
| it(`should return ${expected} for [${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
There was a problem hiding this comment.
Thank you for this. I have read about the matchers for numbers and floats now
There was a problem hiding this comment.
I don't see any change made to the relevant Jest test.
Sprint-1/implement/sum.test.js
Outdated
| // Then it should ignore the non-numerical values and return the sum of the numerical elements | ||
|
|
||
| [ | ||
| { input: ["evan", 3, "mike", 20, 6, "", "/", , , 20], expected: 49 }, |
There was a problem hiding this comment.
Better to explicitly specify undefined instead of leaving the element blank.
cjyuan
added
Reviewed
Ofonimeedak
added
the
Needs Review
Sprint-1/implement/max.js
Outdated
| if (elements.every((e) => typeof e === "string")) return -Infinity; | ||
| const number = []; | ||
| for (let i = 0; i < elements.length; i++) { | ||
| if (typeof elements[i] === "number" && !Number.isNaN(elements[i])) { | ||
| number.push(elements[i]); | ||
| } | ||
| } | ||
| if (number.length === 0) return -Infinity; |
There was a problem hiding this comment.
The code works better without line 3. Can you figure out why?
cjyuan
removed
the
Needs Review
Ofonimeedak
added
the
Needs Review
| [{ input: [], expected: [] }].forEach(({ input, expected }) => | ||
| it(`given an empty array, it returns an empty array [${input}]`, () => { | ||
| expect(dedupe(input)).not.toBe(expected); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
This check is not checking if the return value is an empty array.
| it(`should return same input values [${input}] without duplicate`, () => { | ||
| expect(dedupe(input)).toStrictEqual(expected); | ||
| }) |
There was a problem hiding this comment.
This check cannot check if the function is returning a different array.
| [{ input: [1, 1, 2], expected: [1, 2] }].forEach(({ input, expected }) => | ||
| it("returns a copy not the original array", () => { | ||
| expect(dedupe(input)).toStrictEqual(expected); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
This check is not checking if the function is returning a copy of the original array or not.
cjyuan
removed
the
Needs Review
2 participants
Self checklist
Changelist
I have made changes to the following files, sum.js,include.js and all test cases
I have followed the acceptance criteria for all katas
This PR contains functions for de-duplication and summation of arrays
It also contained a refactored function and different test cases