3
\$\begingroup\$

This function returns the largest product of all elements of a list between the leftmost and rightmost min and max elements. Is there more readable and maybe compact way to rewrite the elif chain in this code:

from operator import mul
from functools import reduce

prod = lambda iterable: reduce(mul, iterable, 1)

def prod_between_min_and_max(arr):
    l_min, r_max = arr.index(min(arr)), len(arr)-1 - arr[::-1].index(max(arr))
    l_max, r_min = arr.index(max(arr)), len(arr)-1 - arr[::-1].index(min(arr))
    if len(range(l_min, r_max+1)) > len(range(l_max, r_min+1)):
        sl = slice(l_min, r_max+1)
    elif len(range(l_min, r_max+1)) < len(range(l_max, r_min+1)):
        sl = slice(l_max, r_min+1)
    elif prod(arr[l_min:r_max+1]) >= prod(arr[l_max:r_min+1]):
        sl = slice(l_min, r_max+1) 
    else:
        sl = slice(l_max, r_min+1)
    return prod(arr[sl])    

than this:

from operator import mul
from functools import reduce

cmp = lambda a, b: (a > b) - (a < b)
prod = lambda iterable: reduce(mul, iterable, 1)

def prod_between_min_and_max(arr):
    l_min, r_max = arr.index(min(arr)), len(arr)-1 - arr[::-1].index(max(arr))
    l_max, r_min = arr.index(max(arr)), len(arr)-1 - arr[::-1].index(min(arr))
    sl = [
        [
            slice(l_max, r_min+1),
            slice(l_min, r_max+1)
        ][
            prod(arr[l_min:r_max+1]) >= prod(arr[l_max:r_min+1])
        ], 
        slice(l_min, r_max+1), 
        slice(l_max, r_min+1)
    ][
        cmp(len(range(l_min, r_max+1)), len(range(l_max, r_min+1)))
    ]
    return prod(arr[sl])
\$\endgroup\$
  • 1
    \$\begingroup\$ I would argue that version two is neither more compact nor more readable. \$\endgroup\$ – jonrsharpe Dec 18 '16 at 23:00
  • \$\begingroup\$ @jonrsharpe, Maybe. I look at the code for so long so I can not see it \$\endgroup\$ – KgOfHedgehogs Dec 18 '16 at 23:16
  • \$\begingroup\$ Is your code compliant with both python 2 and 3? \$\endgroup\$ – dfhwze Aug 3 at 19:57
3
\$\begingroup\$

Slicing lists can be slow, as it performs a copy of the slice in memory, as such, you could write prod like:

def product(iterable, start, stop=None, step=None):
    if stop is None:
        stop = start
        start = 0
    elements = itertools.islice(iterable, start, stop, step)
    return functools.reduce(operator.mul, elements, 1)

You should also define an rindex function that find an element starting from the end of a sequence:

def rindex(sequence, value):
    reverse_enumerate = zip(range(len(sequence) - 1, -1, -1), reversed(sequence))
    return next(i for i, v in reverse_enumerate if v == value)

Using these, you can write your main function:

def product_between_min_and_max(sequence):
    min_element = min(sequence)
    max_element = max(sequence)

    l_min = sequence.index(min_element)
    l_max = sequence.index(max_element)
    r_max = rindex(sequence, max_element)
    r_min = rindex(sequence, min_element)

    if r_max - l_min > r_min - l_max or (r_max - l_min == r_min - l_max
            and product(sequence, l_min, r_max + 1) >= product(sequence, l_max, r_min + 1)):
        sl = (l_min, r_max)
    else:
        sl = (l_max, r_min)
    return product(sequence, sl[0], sl[1] + 1)

You can also simplify ranges by adding 1 at both r_max and r_min since it won't change the result of the tests:

def product_between_min_and_max(sequence):
    min_element = min(sequence)
    max_element = max(sequence)

    l_min = sequence.index(min_element)
    l_max = sequence.index(max_element)
    # Add one to simplify writing the slices
    # Doesn't change (in)equality testings to have +1 on both sides
    r_max = rindex(sequence, max_element) + 1
    r_min = rindex(sequence, min_element) + 1

    if r_max - l_min > r_min - l_max or (r_max - l_min == r_min - l_max
            and product(sequence, l_min, r_max) >= product(sequence, l_max, r_min)):
        sl = l_min, r_max
    else:
        sl = l_max, r_min
    return product(sequence, *sl)
\$\endgroup\$
4
\$\begingroup\$

The second example is terrible, don't use it.


You assign sl to either (l_min, r_max) or (l_max, r_min), and so you just need to change your ifs to become one if and an else.

First off len(range(l_min, r_max+1)) is the same as r_max+1 - l_min, and there's no need for the +1 if both sides have it. So your first if could become: r_max - l_min > r_min - l_max.

To merge the first and third, you should check it's the first, or explicitly check it's not the second and is the third. And so you can get:

def prod_between_min_and_max(arr):
    l_min = arr.index(min(arr))
    l_max = arr.index(max(arr))
    r_max = len(arr)-1 - arr[::-1].index(max(arr))
    r_min = len(arr)-1 - arr[::-1].index(min(arr))
    if (r_max - l_min > r_min - l_max
      or (r_max - l_min == r_min - l_max
          and prod(arr[l_min:r_max+1]) >= prod(arr[l_max:r_min+1]))):
        sl = (l_min, r_max)
    else:
        sl = (l_max, r_min)
    return prod(arr[slice(sl[0], sl[1]+1)])

This reminds me of FizzBuzz, as there doesn't seem to be a 'nice' solution.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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