4
\$\begingroup\$

What is the best way to conditionally use an operator? All 3 methods seemed acceptable to me, and the last 2 better.

If you're curious: I've got a list which is unsorted, and many other lists which may or may not contain a smaller selection of sorted indices referencing the unsorted list. Processed data is being identified as lists which contain sorted references.

I've tried an anonymous function...

if reverse:
    list_is_sorted = lambda unsorted, indices:    \
                     all(unsorted[indices[i]] >=  \
                         unsorted[indices[i + 1]] \   
                         for i in xrange(len(indices) - 1)))        
else:
    list_is_sorted = lambda unsorted, indices:    \ 
                     all(unsorted[indices[i]] <=  \
                         unsorted[indices[i + 1]] \   
                         for i in xrange(len(indices) - 1)))
list_is_sorted()

And an evaluated expression and...

if reverse:
    my_operator = '>='
else:
    my_operator = '<='
eval('all(unsorted[indices[i]] %s  \
          unsorted[indices[i + 1]] \
          for i in xrange(len(indices) - 1))' % my_operator)

Assigning an operator like this:

import operator
if reverse:
    my_operator = lambda x, y: operator.ge(x, y)
else:
    my_operator = lambda x, y: operator.le(x, y)
all(my_operator(unsorted[indices[i]], unsorted[indices[i + 1]]) \
    for i in xrange(len(indices) - 1)))
\$\endgroup\$
  • \$\begingroup\$ First of all, does this code work? If not, then how does it not work? (And is it appropriate for Code Review? \$\endgroup\$ – holroy Oct 11 '15 at 13:35
  • 1
    \$\begingroup\$ Secondly, I do believe the answer to your question is simply to set my_operator = operator.ge if reverse else operator.le... \$\endgroup\$ – holroy Oct 11 '15 at 13:36
  • \$\begingroup\$ @holroy It works. I'm not clear on what would make one of these acceptable or not. They all seem fine to me \$\endgroup\$ – 12345678910111213 Oct 11 '15 at 13:38
8
\$\begingroup\$

I'm a little sure if this is well suited for Code Review, but here goes a little analysis:

Using anonymous functions

This doesn't look right as it is repeating too much code, which is a clear code smell. When quickly looking at it, it is unclear what the actual difference is between the two. So this is not a good option.

Using eval

An old saying says "using eval is evil", and you should avoid using eval if possible. Not only does it have an performance hit, but it is also a major source of security flaws, and not readable code.

Defining your own operator

This is by far the nicest solution, as it doesn't repeat code, and it reads the best. However you could simplify it using direct assignment of the operator instead of using a lambda expression, thusly making it to be:

import operator

my_operator = operator.ge if reverse else operator.le
all(my_operator(unsorted[indices[i]], unsorted[indices[i + 1]]) \
    for i in xrange(len(indices) - 1)))

In a function using a pairwise recipe

Or introducing a function, and the pairwise iterator from itertools recipes v2.7:

import operator
import itertools

def is_sorted(iterable, reverse= False):
    """Check if the iterable is sorted, possibly reverse sorted."""

    def pairwise(iterable):
        """s -> (s0,s1), (s1,s2), (s2, s3), ..."""
        a, b = itertools.tee(iterable)
        next(b, None)
        return itertools.izip(a, b)

    my_operator = operator.ge if reverse else operator.le

    return all(my_operator(current_element, next_element)
                   for current_element, next_element in pairwise(iterable))

# Test the function
print is_sorted([1, 2, 3, 4])            # Should be True
print is_sorted([4, 3, 2, 1], True)      # Should be True
print is_sorted([4, 1, 2, 3])            # Will not be True, ever...
| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

Instead of checking whether the list is sorted in ascending or descending order, you could simply offer to the user the ability to choose whether the list is sorted according to a predicate and make the predicate default to operator.lt. Then your function would be as simple as:

def is_sorted(collection, pred=operator.lt):
    length = len(collection)
    if length < 2:
        return True

    for i in xrange(length - 1):
        if pred(collection[i+1], collection[i]):
            return False

    return True

That way, it even allows users to provide their own custom comparison functions if they need for example a weak order instead of a total order. It makes the function much more powerful without duplicating the code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1 I like the default predicate, allows for less-than not just less-than-or-equal etc \$\endgroup\$ – JBRWilkinson Apr 12 '18 at 12:51
3
\$\begingroup\$

The only difference the flag reverse plays is to change the operator used for comparing things. So the first snippet doesn't make that much sense as it's basically copy/pasting the entire function.

The second snippet is more useful in that regard, as it's only changing the operator in question. Unfortunately it's using eval. Since there are other ways to get a good solution, using eval isn't necessary.

The last snippet is the best one, although it can be shortened a bit:

import operator
compare = operator.ge if reverse else operator.le
all(compare(unsorted[indices[i]], unsorted[indices[i + 1]]) \
    for i in xrange(len(indices) - 1))

I guess there would be some ways to use something from itertools to remove the need for xrange; maybe someone else will post something about that.

There's also the option to use cmp in a slightly different way:

comparison = 1 if reverse else -1
all(comparison * cmp(unsorted[indices[i]], unsorted[indices[i + 1]]) >= 0 \
    for i in xrange(len(indices) - 1))

This works because cmp gives either a negative, zero, or positive integer as a result of comparing the two numbers; with comparison set to either 1 or -1 the direction of the comparison is optionally reversed.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Minor style note, but using \ to split code over lines is advised against. It doesn't look right, and there are other ways to do it. Namely, wrapping things in (), and in one case you're already doing that as well as using the backslash:

 all(unsorted[indices[i]] >=  
     unsorted[indices[i + 1]]    
     for i in xrange(len(indices) - 1)))  

Once they're wrapped in parentheses, Python knows to ignore the newline characters when evaluating the syntax. You could use the same to avoid needing a backslash for your lambda too.

list_is_sorted = lambda unsorted, indices:    (
                 all(unsorted[indices[i]] >=  
                     unsorted[indices[i + 1]]    
                     for i in xrange(len(indices) - 1)))
                 )
| improve this answer | |
\$\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.