Repository navigation
Conversation
…anged other `list` to `sortList`
… input array [3, 1, 2]" working on this test now.
…hout making changed to it using "Array.from(list)"
…nd added in check for .isArray to ensure its an array
…w Set() in the process.
… 20-26 keeps making it look like that
… and another with negative and positive numbers
…ntaining numbers.
was not able to make commit before this save.
…rint and adjust for the code here.
HM-127BTY
added
📅 Sprint 1
hackertainment
added
Review in progress
|
Besides fixing the parts in my feedbacks, would you please also commit the code change to |
hackertainment
added
Reviewed
| if (elements.length === 0) { | ||
| return "Infinity"; | ||
| } |
There was a problem hiding this comment.
elements does not necessary have the .length property/attribute. A better approach can be making sure it is an array first. Please fix it. Thank you.
There was a problem hiding this comment.
Ah I think I understand, my order for it a touch wrong there.
Best to have it an array first then checking the length is the correct order.
| it("returns the Infinity for []", () => { | ||
| expect(findMax([])).toEqual("Infinity"); | ||
| }); |
There was a problem hiding this comment.
It seems not quite accurate to me. Please consider reading line 17 very carefully, and fix the typo in this test case. Thank you.
There was a problem hiding this comment.
Ah I see... its ment to be -Infinity not just Infinity. Fixed that error.
There was a problem hiding this comment.
In JavaScript, Infinity and -Infinity is a number, so that double-quotes (") are not required. If you have double-quote, then it becomes a string and no longer be a number. The advantage of keeping it as a number is that it can be used for numerical calculation (or comparing with other numbers in this case) - like the example logic below:
let largest = -Infinity;
for (const element of elements) {
if (element>largest) {
largest = element;
}
}
return largest;As you may aware, this code segment would look cleaner (and actually faster than the built-in sort() function. Since you can spot the typo yourself, I will let you pass this time - but this is actually an important concept in programming. If you would like to learn more about data type, please book a mentored coding session with a volunteer. Thank you.
There was a problem hiding this comment.
Thank you, I will look into that.
| if (list.length === 0) { | ||
| throw new Error("calculateMean requires a non-empty array"); | ||
| } |
There was a problem hiding this comment.
list does not necessary have the .length property/attribute. A better approach can be making sure it is an array first. Please fix it. Thank you.
There was a problem hiding this comment.
Seen, fixed as same comment as max.js
| if (elements.length === 0) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
elements does not necessary have the .length property/attribute. A better approach can be making sure it is an array first. Please fix it. Thank you.
There was a problem hiding this comment.
Seen, fixed as same comment as max.js
| throw new Error("sum requires an array of numbers"); | ||
| } | ||
|
|
||
| const copyElements = Array.from(elements); |
There was a problem hiding this comment.
For your future reference, copy an array is not needed as your algorithm just reads but does not modify any elements in it.
There was a problem hiding this comment.
Ah I understand, I think i was just copying each time to make sure.
|
Ahhh I see my error, i thought i had worked on it but checking now I missed it. Well get right on it. >.< |
|
Well done and please note my new comment #1487 (comment) . Congratulation, you have completed this PR successfully. |
hackertainment
added
Complete
Learners, PR Template
Self checklist
Task code
CYF-1071
Changelist
Completed code and tests in to meet requirements of the tasks in Sprint-1