Django controller method to list the top characters, guilds, and killers

Question

I've been reading the Chapter 8 of this book.

Here's the view I'm currently coding. It list all characters/guilds according to the type of URL.

/ranking -> List Top 50 Characters
/ranking/guild -> List top guilds
/ranking/pvp -> list top killers

def ranking(request, type):
 if type=='':
 queryset_list = Character.objects.annotate(num_resets=Count('resets')).order_by('-resets').select_related('account')[:50]
 paginator = Paginator(queryset_list, 10)
 page = request.GET.get('page')
 try:
 queryset = paginator.page(page)
 except PageNotAnInteger:
 queryset = paginator.page(1)
 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset, 'states':STATES, 'class_type':CLASS_TYPE}
 template = 'ranking_character.html'
 elif type=='guild':
 queryset_list = Guild.objects.all()
 paginator = Paginator(queryset_list, 10)
 page = request.GET.get('page')
 try:
 queryset = paginator.page(page)
 except PageNotAnInteger:
 queryset = paginator.page(1)
 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset}
 template = 'ranking_guild.html'
 elif type=='pvp':
 queryset_list = Character.objects.filter(pk_count__gt=0).annotate(total=Count('pk_count')).order_by('-pk_count')[:50]
 paginator = Paginator(queryset_list, 10)
 page = request.GET.get('page')
 try:
 queryset = paginator.page(page)
 except PageNotAnInteger:
 queryset = paginator.page(1)
 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset, 'class_type':CLASS_TYPE}
 template = 'ranking_pvp.html'
 return render(request,template,context)

It's possible to make it more "generic"? How can I abstract this different queryset?

Answer 1

Normally the way to refactor is to proceed step by step. For example, you might start with the extract method combined with the decomposition of conditionals.

This would result in:

def _ranking_without_type(request):
 queryset_list = Character.objects.annotate(num_resets=Count('resets')).order_by('-resets').select_related('account')[:50]
 paginator = Paginator(queryset_list, 10)
 page = request.GET.get('page')
 try:
 queryset = paginator.page(page)
 except PageNotAnInteger:
 queryset = paginator.page(1)
 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset, 'states':STATES, 'class_type':CLASS_TYPE}
 template = 'ranking_character.html'
 return context, template
def _ranking_by_guild(request):
 queryset_list = Guild.objects.all()
 paginator = Paginator(queryset_list, 10)
 page = request.GET.get('page')
 try:
 queryset = paginator.page(page)
 except PageNotAnInteger:
 queryset = paginator.page(1)
 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset}
 template = 'ranking_guild.html'
 return context, template
def _ranking_by_pvp(request):
 queryset_list = Character.objects.filter(pk_count__gt=0).annotate(total=Count('pk_count')).order_by('-pk_count')[:50]
 paginator = Paginator(queryset_list, 10)
 page = request.GET.get('page')
 try:
 queryset = paginator.page(page)
 except PageNotAnInteger:
 queryset = paginator.page(1)
 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset, 'class_type':CLASS_TYPE}
 template = 'ranking_pvp.html'
 return context, template
def ranking(request, type):
 if type=='':
 context, template = _ranking_without_type(request)
 elif type=='guild':
 context, template = _ranking_by_guild(request)
 elif type=='pvp':
 context, template = _ranking_by_pvp(request)
 return render(request,template,context)

Run the tests, and check that they still pass. (you have tests, don't you?)

In the next step I would do what Caridorc proposed.

You would have the following code:

def _queryset(request, paginator):
 page = request.GET.get('page')
 try:
 queryset = paginator.page(page)
 except PageNotAnInteger:
 queryset = paginator.page(1)
 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 return queryset
def _ranking_without_type(request):
 queryset_list = Character.objects.annotate(num_resets=Count('resets')).order_by('-resets').select_related('account')[:50]
 paginator = Paginator(queryset_list, 10)
 queryset = _queryset(request, paginator) 
 context = {'object_list' : queryset, 'states':STATES, 'class_type':CLASS_TYPE}
 template = 'ranking_character.html'
 return context, template
def _ranking_by_guild(request):
 queryset_list = Guild.objects.all()
 paginator = Paginator(queryset_list, 10)
 queryset = _queryset(request, paginator) 
 context = {'object_list' : queryset}
 template = 'ranking_guild.html'
 return context, template
def _ranking_by_pvp(request):
 queryset_list = Character.objects.filter(pk_count__gt=0).annotate(total=Count('pk_count')).order_by('-pk_count')[:50]
 paginator = Paginator(queryset_list, 10)
 queryset = _queryset(request, paginator) 
 context = {'object_list' : queryset, 'class_type':CLASS_TYPE}
 template = 'ranking_pvp.html'
 return context, template
def ranking(request, type):
 if type=='':
 context, template = _ranking_without_type(request)
 elif type=='guild':
 context, template = _ranking_by_guild(request)
 elif type=='pvp':
 context, template = _ranking_by_pvp(request)
 return render(request,template,context)

How about refactoring that "template"?

def ranking(request, type):
 template = 'ranking_{0}.html'.format(type if type != '' else 'character')
 if type=='':
 context = _ranking_without_type(request)
 elif type=='guild':
 context = _ranking_by_guild(request)
 elif type=='pvp':
 context = _ranking_by_pvp(request)
 return render(request,template,context)

now the ranking* functions don't return the template anymore, you can remove the second return type from the ranking_* functions.

Finally, you could use a dispatch mapping, e.g., :

ranking_by_types = {
 '': _ranking_without_type,
 'guild': _ranking_by_guild,
 'pvp', _ranking_by_pvp,
} 
def ranking(request, type):
 template = 'ranking_{0}.html'.format(type if type != '' else 'character')
 context = ranking_by_types[type](request)
 return render(request,template,context)

As you can see, functions will be pretty short. IMHO, they should in general not be longer than 10 lines, although some authors claim they should be at most 3 lines long (others up to 20).

• Bruno: Thank you ! Just one more thing. Should I study CBV(Class-Based View) and maybe refactor to it?
• 301_Moved_Permanently: You can't use Paginator(queryset_list, 10) in ranking because you need the queryset_list created in each _ranking* helper functions.
• Markon: @MathiasEttinger thanks, I missed that. Refactoring without testing/running the program is hard :D
• Markon: @Bruno: I think you have multiple ways to refactor that code. However, how many functions ranking* do you have? If they are only 3, well, there is no point in refactoring - I would stop here. However, if they are let's say > 10-15(many!), then I would start thinking how to generalize. As you can see, the central point of the functions is the queryset list. So, you could even further refactor all those queries in a map (they are lazily evaluated, so it's okay). Then, the only changing bit would be the "context". You would have to think about it :)

Answer 2

I think you can just make a function to get the queryset and re-use it:

def queryset_from_page():
 page = request.GET.get('page')
 try:
 return paginator.page(page)
 except PageNotAnInteger:
 return paginator.page(1)
 except EmptyPage:
 return paginator.page(paginator.num_pages)

Also why is context being defined at different indentation levels?

 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset}

vs

 except EmptyPage:
 queryset = paginator.page(paginator.num_pages)
 context = {'object_list' : queryset, 'class_type':CLASS_TYPE}

In one case you define context everytime, other times only when a particular branch in the exception handling is taken: is this a bug?

• Caridorc: I am open to any kind of criticism, @downvoter please leave a comment to explain the problems of this answer.
• Bruno: yes, this is a bug. My intention was to use these variables in the template. thanks!
• jpaugh: @Bruno Moving the try-catch bit into a function is ideal, as Caridorc suggested. Whenever small bugs like the above can escape scrutiny, it is time to refactor!