Является ли эта структура управления запахом кода?

Question

Этот код кажется запах:

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) 

smelliest часть полезность «результата». Кто-нибудь будет любезно понюхать его для меня?

Answer 1

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) 

Минимальная функция сравнивает данные элементы и возвращает минимальное один (конечно: P). Вначале не очевидно, что вы можете контролировать, какое значение используется для их сравнения, это параметр «ключ».

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

Ключевая функция вычисляет «сравнительный ключ», и это то, что используется для сравнения. Ключевая функция по умолчанию - это идентификатор, где ключ сравнения для элемента является самим элементом.

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

Другой пример:

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

Ваш код выше использует item.foo(args) в качестве значений, где элемент приходит из списка; но для сравнения используется результат прохождения через ClassFred.objects.get(arg1=..). Это означает, что конструкция является вашей ключевой функцией:

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) 

Мой код в верхней части просто объединяет это в одном заявлении.

Answer 2

Логика слишком сложна. Трудно прочитать, что вы на самом деле делаете. Упростите этот цикл, вы делаете слишком много там.

Это ИМХО уже довольно неприятный запах.

Answer 3

Вторая до последней линии так долго, что я потерял терпение. Я бы сделал эти две отдельные переменные со значимыми именами, поэтому мы можем понять, что, если предполагается x < y.

Как об этом:

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 4

Итак, я согласен, кажется, что он может быть реорганизован с точки зрения удобочитаемости, но он должен работать. Это было бы мое взятие на нем:

# 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

Это может работать, хотя, я не думаю, что это будет лучше пахнут: р

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