Python control structure utilisation

Question

This code seems to smell:

result = None
for item in list:
    if result is None:
        result = item.foo(args)
    else:
        if ClassFred.objects.get(arg1=result) < ClassFred.objects.get(arg1=item.foo(args)):
            result = item.foo(args)

The smelliest part is the utility of 'result'. Would anyone be kind enough sniff it for me?

Thank you! :)

Answer 1

The logic is too complex. It's difficult to read what you are really doing. Simplify that loop, you are doing too much in there.

This IMHO is already a pretty nasty smell.

Answer 2

The second-to-last line is so long, I lost patience with it. I'd make those two separate variables with meaningful names, so we can figure out what if x < y is supposed to mean.

How about this:

if not list:
    return None

def get_thing(bar):
    # I don't know what this is, so... here:
    return ClassFred.objects.get(arg1=bar.foo(args))

# way less hassle than that if-else stuff
biggest = get_thing(list[0])
for item in list:
    current = get_thing(item)
    if biggest < current:
            biggest = current

Answer 3

L = list # 'list' is a poor variable name, use something else
result = min((n.foo(args) for n in L),
             key=lambda x: ClassFred.objects.get(arg1=x))
# if you don't have to use arg1 as a named parameter:
result = min((n.foo(args) for n in L), key=ClassFred.objects.get)

The min function compares the given items and returns the minimum one (of course :P). What isn't obvious at first is you can control what value is used to compare them, this is the 'key' parameter.

>>> L = [-2, -1, 3]
>>> min(L)
-2
>>> min(L, key=abs)
-1

The key function computes the "comparison key", and that is what is used to compare. The default key function is identity, where the comparison key for an item is the item itself.

>>> def identity(x):
...   return x
>>> min(L, key=identity)
-2

Another example:

>>> min("0000", "11", "222", "3")
"0000" # lexicographical minimum
>>> min("0000", "11", "222", "3", key=len)
"3"

Your code above is using item.foo(args) as values, where item comes from your list; but the result of passing that through ClassFred.objects.get(arg1=..) is used to compare. This means that construct is your key function:

values = (n.foo(args) for n in L) # this is a generator expression
# it is similar to a list comprehension, but doesn't compute or store
# everything immediately

def keyfunc(x):
  return ClassFred.objects.get(arg1=x)

result = min(values, key=keyfunc)

My code at the top just puts this together in one statement.

Answer 4

So, I agree it seems like it could be refactored from a readability standpoint, but it should work. This would be my take on it:

# using lst instead of list & at least python 2.5
result = lst[0].foo(args) if lst else None
fxn = ClassFred.objects.get

for item in lst[1:]:
    if fxn(arg1=result) > fxn(arg1=item.foo(args)):
        result = item.foo(args)

Answer 5

This may work, though, I don't think it'll smell better :p

max([ClassFred.objects.get(arg1=item.foo(args)), item.foo(args) for item in list])[1]