Skip to content

improve findmin#870

Closed
RuSaG0 wants to merge 1 commit intoTheAlgorithms:masterfrom
RuSaG0:min
Closed

improve findmin#870
RuSaG0 wants to merge 1 commit intoTheAlgorithms:masterfrom
RuSaG0:min

Conversation

@RuSaG0
Copy link
Contributor

@RuSaG0 RuSaG0 commented Dec 9, 2021

improve find Min method
if no params to calc min => it's not error, it's undefined/null
work with generators, custom selector fn

@RuSaG0 RuSaG0 requested a review from raklaptudirm as a code owner December 9, 2021 15:14
@RuSaG0 RuSaG0 changed the title findmin improve findmin 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.

Please explain how your implementation is better. I for one, think the previous one was the better one.

@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

If the goal is learning and understanding what the search for the minimum is - the previous implementation is better (although, as it seems to me, everyone already understands what the search for the minimum means)

If we need a more 'production-ready function' that is able to work with different structures (iterables for example) and with different ways to search for the minimum itself, then mine is better. Finding the minimum is not always looking for the smallest number

@raklaptudirm
Copy link
Member

This repo is not for production ready code, but for self explanatory code understandable for beginners of the programming language. Throwing iterators at them is not a good idea imo.

@trasherdk
Copy link
Contributor

trasherdk commented Dec 23, 2021

I like this implementation.

Maybe as an alternative FindMinIterator() or something.

  test('given array of objects then min accessor is found', () => {
    const array = [
      { name: 'Item #1', price: 1.0 },
      { name: 'Item #2', price: 0.0 },
      { name: 'Item #3', price: -1.0 }
    ]
    expect(findMin(array, _x => _x.price)).toBe(-1)
    expect(findMin(array, _x => _x.name)).toBe('Item #1')
  })

The tests pass

$ npx jest --no-cache Maths/test/FindMin.test.js 
 PASS  Maths/test/FindMin.test.js
  findMin
    ✓ given empty array then min is undefined (2 ms)
    ✓ given single value array then min is found (1 ms)
    ✓ given array then min is found
    ✓ given empty generator then min is undefined
    ✓ given generator then min is found
    ✓ given string generator then min string length is found
    ✓ given array of objects then min accessor is found (1 ms)

Test Suites: 1 passed, 1 total
Tests:       7 passed, 7 total
Snapshots:   0 total
Time:        1.147 s
Ran all test suites matching /Maths\/test\/FindMin.test.js/i.

Edit: Expanded the test a bit.

@booleans-oss
Copy link

This repo is not for production ready code, but for self explanatory code understandable for beginners of the programming language. Throwing iterators at them is not a good idea imo.

I agree. Iterators and generators are really difficult topics to understand and even more to explain. I think it is far more easier and useful to explain a simple for-loop (as seen in the previous implementation) than explaning iterators. So in my opinion and for the target audience of this repo, I think the previous implementation is better.

@raklaptudirm
Copy link
Member

@trasherdk I completely agree on the fact that this implementation is faster and more useful than the current one, but thats not my point.

@trasherdk
Copy link
Contributor

@raklaptudirm I get your point. Don't complicate stuff 😃

I just liked the implementation, and the use case I demonstrated.

@raklaptudirm
Copy link
Member

We could add this as a separate implementation though.

@trasherdk
Copy link
Contributor

That would work too 😃

What do you say @RuSaG0 ?

@RuSaG0
Copy link
Contributor Author

RuSaG0 commented Mar 1, 2022

@trasherdk
yes, let's make it as second method (as you say "FindMinIterator()")

@raklaptudirm
Copy link
Member

@RuSaG0 Make the changes and I will review them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on hold Being discussed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants