user1504605 user1504605 - 4 years ago 156
Python Question

How can I write this with less redundancy/copy-paste?

I usually don't write my Python code in the best way since I'm relatively new to it, someone requested that I make changes to a Django app since the code doesn't look so nice.

Here's what it looks like:

@login_required
def submission_set_rank(request):

r1_obj_id = request.GET.get('rank1','')
r2_obj_id = request.GET.get('rank2','')
r3_obj_id = request.GET.get('rank3','')
r4_obj_id = request.GET.get('rank4','')
r5_obj_id = request.GET.get('rank5','')

#rate the first BallotStats object
ballot_1 = BallotStats.objects.get(object_id=r1_obj_id)
ballot_2 = BallotStats.objects.get(object_id=r2_obj_id)
ballot_3 = BallotStats.objects.get(object_id=r3_obj_id)
ballot_4 = BallotStats.objects.get(object_id=r4_obj_id)
ballot_5 = BallotStats.objects.get(object_id=r5_obj_id)

ballot_1.score += 5
ballot_2.score += 4
ballot_3.score += 3
ballot_4.score += 2
ballot_5.score += 1

ballot_1.save()
ballot_2.save()
ballot_3.save()
ballot_4.save()
ballot_5.save()

return HttpResponseRedirect('/submissions/results/film/')


As it turns out I realized that I've always been writing my Python code this way, is there a way to make it look better instead of taking up 21+ lines of code?

Answer Source

The biggest problem is not the style of the code - it is that you are making 10 queries: 5 for getting the objects and 5 for updating them.

Filter out objects using __in at once:

@login_required
def submission_set_rank(request):   
    keys = {'rank1': 5, 'rank2': 4, 'rank3': 3, 'rank4': 2, 'rank5': 1}
    ranks = [request.GET.get(key,'') for key in keys]
    for ballot in BallotStats.objects.filter(object_id__in=ranks):
        ballot.score += keys[ballot.object_id]
        ballot.save()

    return HttpResponseRedirect('/submissions/results/film/')

This will make 6 queries at most: 1 for getting the objects and 5 for updating them.

Also, you can "mark" the view with the commit_manually decorator (commit_on_success would also work for you). It should speed up things significantly:

@login_required
@transaction.commit_manually
def submission_set_rank(request):   
    keys = {'rank1': 5, 'rank2': 4, 'rank3': 3, 'rank4': 2, 'rank5': 1}
    ranks = [request.GET.get(key,'') for key in keys]
    for ballot in BallotStats.objects.filter(object_id__in=ranks):
        ballot.score += keys[ballot.object_id]
        ballot.save()
    transaction.commit()

    return HttpResponseRedirect('/submissions/results/film/')

And I have the strong feeling that you can do this in even a single update query. For example, by using connection.cursor() directly with the help of executemany():

@login_required
def submission_set_rank(request):   
    keys = {'rank1': 5, 'rank2': 4, 'rank3': 3, 'rank4': 2, 'rank5': 1}
    ranks = [{'score': request.GET.get(key,''), 'id': key} for key in keys]

    cursor = connection.cursor()
    cursor.executemany("""
        UPDATE
            ballot_stats
        SET
            score = score + %(score)s
        WHERE
            object_id = %(id)s
    """, ranks)

    return HttpResponseRedirect('/submissions/results/film/')

Make sure the field and table names are correct.

Recommended from our users: Dynamic Network Monitoring from WhatsUp Gold from IPSwitch. Free Download