London|ITP-September-2025|Mariia Serhiienko|Sprint 1|Implement #896
Conversation
…js and max.test.js
…js and sum.test.js
…pe.js and dedupe.test.js
Konvaly
added
the
Needs Review
wheresdiasd
added
Review in progress
Sprint-1/fix/median.js
Outdated
| } | ||
|
|
||
| const filteredNumericArr = list.filter( | ||
| (el) => typeof el === "number" && Number.isFinite(el) |
There was a problem hiding this comment.
According to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isFinite
isFinite already checks if the element is a number ( typeof el === "number" ).
I like the usage of the method tho.
There was a problem hiding this comment.
Thank you for pointing this ! I definetely missed this, so now fixed.
| const sortedArr = [...filteredNumericArr].sort((a, b) => a - b); | ||
| const middleIndex = Math.floor(sortedArr.length / 2); | ||
|
|
||
| if (sortedArr.length % 2 !== 0) { |
There was a problem hiding this comment.
Why do we need this last logic from 24 to 28 lines?
There was a problem hiding this comment.
Because when the list has an even number of numbers, there isn’t just one middle value, but there are two. In that situation, the median is defined as the average of those two middle numbers.
Sprint-1/implement/sum.js
Outdated
| (currMax, el) => Math.max(currMax, (String(el).split(".") || "").length), | ||
| 0 | ||
| ); | ||
| const decMultiplier = 10 ** maxDecimalPlaces; |
There was a problem hiding this comment.
This is a very robust solution, but too complex for the sake of the learning purposes, could we assume that the max float decimal places are 2 ? as we see in tests ?
There was a problem hiding this comment.
Yes, you are right! I really did this too complicated, becouse tried overthinking about cases with max float decimal points more than 2.
Sprint-1/implement/sum.js
Outdated
|
|
||
| const total = numbers.reduce((acc, el) => acc + el, 0); | ||
|
|
||
| return Math.round((total + Number.EPSILON) * decMultiplier) / decMultiplier; |
There was a problem hiding this comment.
Therefore as a consequence of the comment above, we will not need Epsilon here as MathRound will do its work as expected.
There was a problem hiding this comment.
Makes sense! Since we’re only dealing with 2 decimal places now, so I’ve removed it to keep things clean and simple.
I’ll keep in mind not to overcomplicate things next time!
wheresdiasd
added
Reviewed
Konvaly
added
Needs Review
wheresdiasd
added
Complete
|
Thank you @wheresdiasd ! |
3 participants
Self checklist
Added tests and implemented solution for the fix, implement and refactor sections