Skip to content

Conversation

@Konvaly
Copy link

@Konvaly Konvaly commented Nov 29, 2025

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Added tests and implemented solution for the fix, implement and refactor sections

@Konvaly Konvaly added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 29, 2025
@wheresdiasd wheresdiasd self-requested a review November 29, 2025 20:28
@wheresdiasd wheresdiasd added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 29, 2025
}

const filteredNumericArr = list.filter(
(el) => typeof el === "number" && Number.isFinite(el)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const sortedArr = [...filteredNumericArr].sort((a, b) => a - b);
const middleIndex = Math.floor(sortedArr.length / 2);

if (sortedArr.length % 2 !== 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this last logic from 24 to 28 lines?

(currMax, el) => Math.max(currMax, (String(el).split(".") || "").length),
0
);
const decMultiplier = 10 ** maxDecimalPlaces;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

https://github.com/CodeYourFuture/Module-Data-Groups/pull/896/files#diff-b88d7772fc056e6e9f3a6eb58f5cc851f9187a7f26f8f979a80e070bf79bf1bdR38


const total = numbers.reduce((acc, el) => acc + el, 0);

return Math.round((total + Number.EPSILON) * decMultiplier) / decMultiplier;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore as a consequence of the comment above, we will not need Epsilon here as MathRound will do its work as expected.

@wheresdiasd wheresdiasd added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Dec 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants