6
\$\begingroup\$

I'm reimplementing the min() function as an exercise (EDIT: not all the functionality of the python std library function, just the minimum of a list of numbers). Here is my code:

def my_min(num_list):
    minimum = num_list[0]
    for num in num_list[1:]:
        if num < minimum:
            minimum = num            
    return minimum

My question is: How bad is num_list[1:] in the for loop? And are there any other optimizations I could make to the code?

My intention by truncating the list is to avoid comparing the list's first element to itself. While insignificant in terms of wasted time and resources, I just find it lacking elegance.

\$\endgroup\$
8
  • \$\begingroup\$ what happens if num_list has only 1 element? \$\endgroup\$
    – dfhwze
    Aug 13, 2019 at 19:35
  • 3
    \$\begingroup\$ If the purpose is to comply to the specification of min, you are far away from home: programiz.com/python-programming/methods/built-in/min. \$\endgroup\$
    – dfhwze
    Aug 13, 2019 at 19:38
  • 1
    \$\begingroup\$ @dfhwze, good point, I edited my question \$\endgroup\$ Aug 13, 2019 at 19:45
  • 2
    \$\begingroup\$ It does reallocate, yes. Expressions evaluate in the same way whether you assign them to a variable or not. If you want to iterate starting from a different index, you’d want to either iterate with indices manually (for i in range(1, len(num_list))) or advance an iterator manually (it = iter(xs); minimum = next(it); for x in it: ...). The latter will also work on containers that don’t support slicing. \$\endgroup\$
    – wchargin
    Aug 14, 2019 at 6:52
  • 1
    \$\begingroup\$ What happens if num_list has zero elements? \$\endgroup\$
    – Graipher
    Aug 14, 2019 at 7:20

2 Answers 2

11
\$\begingroup\$

iterators

You can use an iterator

def my_min(num_list):
    # empty lists
    if not num_list:
        raise ValueError('Empty list')

    list_iter = iter(num_list)
    minimum = next(list_iter)
    for num in list_iter:
        if num < minimum:
            minimum = num            
    return minimum

In response to Mathias' comment, here is a version that works with an iterable:

def my_min(seq):
    seq = iter(seq)

    try:
        minimum = next(seq)

        for num in seq:
            if num < minimum:
                minimum = num            

        return minimum

    except StopIteration as e:
        pass

    raise ValueError('Empty list')

Improved based on @Wombatz comment:

def my_min(seq):
    seq = iter(seq)

    try:
        minimum = next(seq)

    except StopIteration as e:
        raise ValueError('Empty list') from None

    else:
        for num in seq:
            if num < minimum:
                minimum = num            

        return minimum
\$\endgroup\$
4
  • 3
    \$\begingroup\$ This is the right approach to avoid duplicating the whole list in memory. If only you took the special case of the empty parameter around the next call, you could handle all kind of iterables, not only lists. \$\endgroup\$ Aug 13, 2019 at 21:36
  • \$\begingroup\$ @MathiasEttinger good idea. Added it to the answer. \$\endgroup\$
    – RootTwo
    Aug 13, 2019 at 22:22
  • \$\begingroup\$ Wouldn't it be nicer to only wrap the line minimum = next(seq) in the try block as only that line can actually raise the StopIteration?. Then in the except block you could raise ValueError(...) from None. \$\endgroup\$
    – Wombatz
    Aug 14, 2019 at 15:01
  • \$\begingroup\$ @Wombatz That's how I coded it at first, but I didn't like that the printed traceback had the StopIteration as well as the ValueError. I didn't know about the from clause of the raise statement. It's a good day when you learn something. \$\endgroup\$
    – RootTwo
    Aug 14, 2019 at 17:03
6
\$\begingroup\$

Review

I should check for the list having no elements anyway, so I could check for its having only 1 element and return that element

  • You have implemented a simple function, so it shouldn't have been that hard to provide a couple of unit tests. You would have immediately found bugs on the most obvious edge cases as (1) empty list and self-created edge case (2) single item.

While insignificant in terms of wasted time and resources, I just find it lacking elegance.

  • What you gain in elegance is lost by the edge case guards you'd have to build in to fix the bugs.
\$\endgroup\$
3
  • 1
    \$\begingroup\$ To your second point, are you saying there's no way to get the best of both worlds and therefore comparing the first element to itself is the way to go in terms of readability of code, etc...? \$\endgroup\$ Aug 13, 2019 at 20:07
  • 1
    \$\begingroup\$ If you don't want to check the first element against itself, you would always need to build in some check whether the array has more than one element. I believe checking the first element against itself does not harm readability, performance or elegance. \$\endgroup\$
    – dfhwze
    Aug 14, 2019 at 4:24
  • 2
    \$\begingroup\$ Thanks for clarifying, I can see that the extra check would be worse of course. I guess the useless comparison just hurts the ocd in me. \$\endgroup\$ Aug 14, 2019 at 4:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.