current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
1

I wrote a messy function and I am wondering if you see any way I could clean it up. Essentially it takes in a list e_1=2&e_2=23&e_3=1 and makes a queryset out of it, maintaining the ordering.

from operator import itemgetter
def ids_to_students(items, prefix=0):

    if prefix == 0:
        # Make ordered QuerySets of Students based on there ids
        etuples   = sorted([(k,v) for k,v in items if k[:2] == 'e_'], key=itemgetter(0))
        ituples   = sorted([(k,v) for k,v in items if k[:2] == 'i_'], key=itemgetter(0))
        tuples = etuples+ituples
    else:
        tuples   = sorted([(k,v) for k,v in items if k[:2] == '%s_'%prefix], key=itemgetter(0))

    pk_list  = [v for (k,v) in tuples]
    clauses   = ' '.join(['WHEN id=%s THEN %s' % (pk, i) for i, pk in enumerate(pk_list)])
    ordering  = 'CASE %s END' % clauses
    students = Student.objects.filter(pk__in=pk_list).extra(
                select={'ordering': ordering}, order_by=('ordering',))
    return students

It's called like this:

students = ids_to_students(request.GET.items())
e_students = ids_to_students(request.GET.items(), 'e')
share|improve this question

2 Answers 2

up vote 5 down vote accepted

Tuples are sorted primarily by the first item of each. If they are all unique, specifying key=itemgetter(0) makes no difference.

This code

if prefix == 0:
    etuples   = sorted([(k,v) for k,v in items if k[:2] == 'e_'], key=itemgetter(0))
    ituples   = sorted([(k,v) for k,v in items if k[:2] == 'i_'], key=itemgetter(0))
    tuples = etuples+ituples
else:
    tuples   = sorted([(k,v) for k,v in items if k[:2] == '%s_'%prefix], key=itemgetter(0))

can be rearranged like this to avoid repetition:

prefixes = ['%s_'%prefix] if prefix else ['e_', 'i_']
tuples = sorted(item for item in items if item[0][:2] in prefixes)
share|improve this answer
    
Thanks! That is a lot cleaner. (upvoted :) –  broinjc Mar 21 '14 at 22:46
    
Great improvement! I used your improvement in final code of my answer, hope you do not mind. –  Ruslan Osipov Mar 22 '14 at 4:31
up vote 5 down vote

In addition to logic, which Janne's answer covered, here is some style analysis (which is an important part of writing good code):

First - it's nice to have a good docstring, with all the information about the function - what does it do, what does it take, what it returns.

from operator import itemgetter

This can be a matter of opinion, but it's a good practice not to import member of a module. So doing import operator and calling operator.itemgetter in your module might give someone who is reading your code a better idea of where the itemgetter is coming from.

def ids_to_students(items, prefix=0):

Since prefix is a string, it might be more clear to provide default value as ''.

etuples   = sorted([(k,v) for k,v in items if k[:2] == 'e_'], key=itemgetter(0))

Only one space before and after =, missing space after comma.

tuples = etuples+ituples

One space before and after + sign.

tuples   = sorted([(k,v) for k,v in items if k[:2] == '%s_'%prefix], key=itemgetter(0))

This line is longer then 80 characters, which is in most cases a very reasonable limit. Breaking it down into two is a good idea.

students = Student.objects.filter(pk__in=pk_list).extra(
            select={'ordering': ordering}, order_by=('ordering',))

When you break the statement into two lines - indent the second line by the multiple of 4.

And the whole thing (I took the list comprehension from Janne's answer, don't forget to upvote it):

def ids_to_students(items, prefix=''):
    """Converts request items into a queryset.

    Usage:
        >>> students = ids_to_students(request.GET.items())
        >>> e_students = ids_to_students(request.GET.items(), 'e')

    Args:
        items: A dict of request.GET.items().
        prefix: A prefix for DB entries.

    Returns:
        A queryset.
    """
    # Make ordered QuerySets of Students based on their ids.
    prefixes = ['%s_' % prefix] if prefix else ['e_', 'i_']
    tuples = sorted(item for item in items if item[0][:2] in prefixes)

    pk_list = [v for (k, v) in tuples]
    clauses = ' '.join(['WHEN id=%s THEN %s' % (pk, i) for i, pk in enumerate(pk_list)])
    ordering = 'CASE %s END' % clauses
    students = Student.objects.filter(pk__in=pk_list).extra(
            select={'ordering': ordering}, order_by=('ordering', ))
    return students
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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