1
vote
\$\begingroup\$

Is there a better (more pythonic?) way to filter a list on attributes or methods of objects than relying on lamda functions?

contexts_to_display = ...
tasks = Task.objects.all()
tasks = filter(lambda t: t.matches_contexts(contexts_to_display), tasks)
tasks = filter(lambda t: not t.is_future(), tasks)
tasks = sorted(tasks, Task.compare_by_due_date)

Here, matches_contexts and is_future are methods of Task. Should I make those free-functions to be able to use filter(is_future, tasks)?

Any other comment?

\$\endgroup\$

locked by Jamal Nov 7 '16 at 23:59

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • 3
    \$\begingroup\$ I think this is too specific a question to qualify as a code review. \$\endgroup\$ – sepp2k Feb 1 '11 at 22:49
  • \$\begingroup\$ Are you coding Django? Looks like it from the Task.objects.all() line. \$\endgroup\$ – Nikolai Prokoschenko Feb 10 '11 at 16:47
  • \$\begingroup\$ That's part of the Django app I'm writing to learn both Python and Django, yes... \$\endgroup\$ – Xavier Nodet Feb 10 '11 at 17:03
7
votes
\$\begingroup\$

I would use a list comprehension:

contexts_to_display = ...
tasks = [t for t in Task.objects.all()
         if t.matches_contexts(contexts_to_display)
         if not t.is_future()]
tasks.sort(cmp=Task.compare_by_due_date)

Since you already have a list, I see no reason not to sort it directly, and that simplifies the code a bit.

The cmp keyword parameter is more of a reminder that this is 2.x code and will need to be changed to use a key in 3.x (but you can start using a key now, too):

import operator
tasks.sort(key=operator.attrgetter("due_date"))
# or
tasks.sort(key=lambda t: t.due_date)

You can combine the comprehension and sort, but this is probably less readable:

tasks = sorted((t for t in Task.objects.all()
                if t.matches_contexts(contexts_to_display)
                if not t.is_future()),
               cmp=Task.compare_by_due_date)
\$\endgroup\$
  • \$\begingroup\$ Thanks for the tip about list comprehension! As compare_by_due_date specifically handles null values for the due date, I'm not sure I can use a key. But I'll find out! \$\endgroup\$ – Xavier Nodet Feb 2 '11 at 18:20
  • \$\begingroup\$ @XavierNodet: Anything which can be compared by a cmp parameter can be converted (even if tediously) to a key by simply wrapping it. With my use of the "due_date" attribute, I'm making some assumptions on how compare_by_due_date works, but if you give the code for compare_by_due_date (possibly in a SO question?), I'll try my hand at writing a key replacement for it. \$\endgroup\$ – Fred Nurk Feb 2 '11 at 18:24
  • \$\begingroup\$ Thanks! SO question is here: stackoverflow.com/q/4879228/4177 \$\endgroup\$ – Xavier Nodet Feb 2 '11 at 20:00
3
votes
\$\begingroup\$

Since you are writing Django code, you don't need lambdas at all (explanation below). In other Python code, you might want to use list comprehensions, as other commenters have mentioned. lambdas are a powerful concept, but they are extremely crippled in Python, so you are better off with loops and comprehensions.

Now to the Django corrections.

tasks = Task.objects.all()

tasks is a QuerySet. QuerySets are lazy-evaluated, i.e. the actual SQL to the database is deferred to the latest possible time. Since you are using lambdas, you actually force Django to do an expensive SELECT * FROM ... and filter everything manually and in-memory, instead of letting the database do its work.

contexts_to_display = ...

If those contexts are Django model instances, then you can be more efficient with the queries and fields instead of separate methods:

# tasks = Task.objects.all()
# tasks = filter(lambda t: t.matches_contexts(contexts_to_display), tasks)    
# tasks = filter(lambda t: not t.is_future(), tasks)
# tasks = sorted(tasks, Task.compare_by_due_date)
qs = Task.objects.filter(contexts__in=contexts_to_display, date__gt=datetime.date.today()).order_by(due_date)
tasks = list(qs)

The last line will cause Django to actually evaluate the QuerySet and thus send the SQL to the database. Therefore you might as well want to return qs instead of tasks and iterate over it in your template.

\$\endgroup\$
  • \$\begingroup\$ Context is indeed a model class, but tasks may have no context, and I want to be able to retrieve the set of all tasks that 'have a given context or no context'. But context__in=[c,None] does not work... Maybe 'no context' should actually be an instance of Context... \$\endgroup\$ – Xavier Nodet Feb 11 '11 at 14:25
  • \$\begingroup\$ Similarly, the date may be null, so I can't simply filter or order on the date... \$\endgroup\$ – Xavier Nodet Feb 11 '11 at 14:26
2
votes
\$\begingroup\$

I think lambdas are fine in this case. (Yeah, not much of a code review, but what can I say... You basically ask a yes/no question. Answer: "No". :) )

\$\endgroup\$
0
votes
\$\begingroup\$

The first lambda (calling matches_contexts) can't be avoided because it has to capture the contexts_to_display, but the not is_future() can be moved into a new Task method can_start_now: it's clearer (hiding the negative conditions), reusable, and this condition will most probably be more complicated in the future. Yes, YAGNI, I know... ;)

And because I did not need the sorting phase to return a copy of tasks, I used in-place sort. By the way, the arguments are reversed between filter(f,iterable) and sorted(iterable,f), using one just after the other seemed akward...

So the code is now:

class Task:
    ...
    def can_start_now(self):
        return not self.is_future()


contexts_to_display = ...
tasks = Task.objects.all()
tasks = filter(lambda t: t.matches_contexts(contexts_to_display), tasks)
tasks = filter(Task.can_start_now, tasks)
tasks.sort(Task.compare_by_due_date)
\$\endgroup\$

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