Skip to content

improve median #869

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

Merged
merged 4 commits into from
Dec 20, 2021
Merged

improve median #869

merged 4 commits into from
Dec 20, 2021

Conversation

RuSaG0
Copy link
Contributor

@RuSaG0 RuSaG0 commented Dec 9, 2021

Describe your change:

Fix a bug
In the last implementation, finding the median is not a pure function. It mutates the original array.

const arr = [3,2,1]
averageMedian(arr)
numbers = numbers.sort(sortNumbers)
so arr will be [1,2,3].

Why? The function for finding the median should not change arr by sorting

====
Now this fn can work with generators and custom median selector (for example of str lengths)

@RuSaG0 RuSaG0 requested a review from raklaptudirm as a code owner December 9, 2021 15:02
@RuSaG0 RuSaG0 changed the title median improve median Dec 9, 2021
Copy link
Member

@raklaptudirm raklaptudirm left a comment

Choose a reason for hiding this comment

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

Any iterables can be converted to an array by simply calling Array.from(iterator). I believe the previous implementation was more concise and to the point.

If you want a pure function, I would suggest a "rest" argument for the function.

@raklaptudirm raklaptudirm added the on hold Being discussed by the maintainers label Dec 11, 2021
@RuSaG0
Copy link
Contributor Author

RuSaG0 commented Dec 19, 2021

'I believe the previous implementation was more concise and to the point' - no problem
I returned the previous one, only now the passed array doesn't change

===
updated

@raklaptudirm raklaptudirm added changes required This pull request needs changes feature Adds a new feature Reviewed and removed on hold Being discussed by the maintainers labels Dec 20, 2021
@raklaptudirm raklaptudirm removed the changes required This pull request needs changes label Dec 20, 2021
@raklaptudirm raklaptudirm merged commit 49fa7d1 into TheAlgorithms:master Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants