-
-
Notifications
You must be signed in to change notification settings - Fork 211
Glasgow | 25-ITP-Sep | Abraham-Habte | Sprint 1 | Sprint1-exercise #811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is good. I only have few suggestions.
| // Then it should return a copy of the original array | ||
| test("given an array with no duplicates", () => { | ||
| const input = [1, 2, 3, 4, 5]; | ||
| const result = dedupe(input); | ||
| expect(result).toEqual([1, 2, 3, 4, 5]); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current test code cannot check if the returned array is a copy of the original array because toEqual() compares objects (including arrays) by value. To illustrate,
const A = [2, 3, 1];
const B = [...A]; // B is a copy of A
// This set of code cannot distinguish if the compared objects are the same objects.
expect(A).toEqual(A); // true
expect(A).toEqual(B); // true
In order to check if the returned array is a copy of the original array, we would need additional checks.
Can you find out what code you need to add in order to ensure the returned value is not the original array?
Sprint-1/implement/sum.js
Outdated
| function sum(elements) { | ||
| } | ||
| if (!Array.isArray(elements)) return null | ||
| const sumNum = elements.filter(n => typeof n === 'number' && !isNaN(n)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you were required to filter out also Infinity and -Infinity, how would you express the filtering condition?
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function dedupe(groupNumber) { | ||
| return [...new Set(groupNumber)]; | ||
| const seen = new Set() | ||
| const deduped = [] | ||
|
|
||
| for (const item of groupNumber) { | ||
| if (!seen.has(item)) { | ||
| seen.add(item) | ||
| deduped.push(item) | ||
| } | ||
| } | ||
|
|
||
| return deduped | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why replaced the previous single-line statement with 10 lines code?
Sprint-1/implement/sum.js
Outdated
| if (!Array.isArray(elements)) return null | ||
| const sumNum = elements.filter(n => typeof n === 'number' && !isNaN(n)) | ||
| return sumNum.reduce((accumulator, currentValue) => accumulator + currentValue, 0); | ||
| }; | ||
|
|
||
| return elements.reduce((accumulator, currentValue) => { | ||
| if (typeof currentValue === "number") { | ||
| return accumulator + currentValue; | ||
| } | ||
| return accumulator; | ||
| }, 0); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rewrote the code?
|
I wasn't asking you to change code. Can you respond to my questions? |
|
I am sorrry which question did I didn't respond |
|
yes I got it now sorry for not responding properly. in sum.js I rewrote the code because I thought that if there is a value wich is nan the sum,test.js will throw an error but when i notice that the tests are writen to ignor the NAN and add the Number there fore i writern to my first code and this code ignor the NAN. by the .filter() in which only number are included in the summation |
| // Then it should return a copy of the original array | ||
| test("given an array with no duplicates", () => { | ||
| const input = [1, 2, 3, 4, 5]; | ||
| const result = dedupe(input); | ||
| expect(result).toStrictEqual([1, 2, 3, 4, 5]); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A is a copy of B" means A and B refer to two different arrays.
The check you have does not check if result and input are two different arrays; it only checks if
result and [1,2,3,4,5] have the same type and elements.
Suggestion: look up "How to check if two arrays are different arrays with the same content in Jest?"
Sprint-1/implement/sum.js
Outdated
| if (typeof currentValue === "number" && Number.isFinite(currentValue)) { | ||
| return accumulator + currentValue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the condition can further simplified.
| const input = [1, 2, 3, 4, 5]; | ||
| const result = dedupe(input); | ||
| expect(result).toStrictEqual([1, 2, 3, 4, 5]); | ||
| expect(result).not.toBe([1, 2, 3, 4, 5]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax [1, 2, 3, 4, 5] always creates a new array.
On line 32, the statement is not checking if result and the array passed to dedupe() are different arrays.


Learners, PR Template
Self checklist
Changelist
I have done all the tasks required for this sprint
Questions
No questions