London | May-2025 | Halimatou Saddiyaa | Module-Data-Groups | Sprint 1 #548
Conversation
Halimatou-saddiyaa
added
📅 Sprint 1
a-robson
added
Review in progress
Sprint-1/fix/median.js
Outdated
| // Secondly, we need to implement a function that filter out non numeric values. | ||
| const numbers = list.filter((item) => typeof item === "number"); | ||
|
|
There was a problem hiding this comment.
Excellent work. However there is an edge case that the tests don't require you to handle. You might want to look at the special NaN property and consider whether the code would work if this value appeared in one of the test arrays. As I say it is not required but might be something to watch out for in the future.
There was a problem hiding this comment.
Thank you, the code might not work with Nan as although it is a number it's not a valid numeric value for math operations.
I've corrected my function to include the case of NaN and added a test to check.
Sprint-1/implement/dedupe.js
Outdated
| function dedupe(array) { | ||
| const seen = new Set(); | ||
| return array.filter((item) => { | ||
| if (seen.has(item)) { | ||
| return false; | ||
| } | ||
| seen.add(item); | ||
| return true; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Clever solution. Using a Set is the right approach in most de-duplication problems. However there are easier ways to populate a Set from an Array. Do you need to use the filter function at all?
There was a problem hiding this comment.
Thanks, I don't need to use the filter function as I can directly use Set and populate it from an Array. I've corrected my code to reflect that.
| function findMax(arr) { | ||
| const numbers = arr.filter((item) => typeof item === "number"); | ||
| if (numbers.length === 0) { | ||
| return -Infinity; | ||
| } | ||
| return Math.max(...numbers); | ||
| } |
Sprint-1/implement/sum.js
Outdated
| function sum(arr) { | ||
| return arr.reduce((total, current) => { | ||
| if (typeof current === "number") { | ||
| return total + current; | ||
| } | ||
| return total; | ||
| }, 0); | ||
| } |
There was a problem hiding this comment.
This is good. However it would be more typical pattern to use filter then reduce rather than having your 'filtering' logic within the reduce function. Why do you think that pattern would be preferred?
There was a problem hiding this comment.
Thanks, I think using filter then reduce will be preferred because it separates the two steps and the code is then easier to read and understand. I've updated my code to reflect that.
a-robson
added
Complete
|
Thank you for reviewing my PR @a-robson |
|
You're welcome. I've looked over your changes and they are all really good. |
Learners, PR Template
Self checklist
Changelist
I've ompleted the following coursework on Sprint 1 of the Module Data Groups:
Please review my PR
Questions
Ask any questions you have for your reviewer.