6
\$\begingroup\$

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?

\$\endgroup\$
2
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ Thank you ! Just one more thing. Should I study CBV(Class-Based View) and maybe refactor to it? \$\endgroup\$ – Bruno Jun 1 '16 at 16:25
  • \$\begingroup\$ You can't use Paginator(queryset_list, 10) in ranking because you need the queryset_list created in each _ranking* helper functions. \$\endgroup\$ – 301_Moved_Permanently Jun 1 '16 at 18:19
  • \$\begingroup\$ @MathiasEttinger thanks, I missed that. Refactoring without testing/running the program is hard :D \$\endgroup\$ – Markon Jun 2 '16 at 9:10
  • 1
    \$\begingroup\$ @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 :) \$\endgroup\$ – Markon Jun 2 '16 at 9:17
2
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ I am open to any kind of criticism, @downvoter please leave a comment to explain the problems of this answer. \$\endgroup\$ – Caridorc Jun 1 '16 at 14:08
  • 2
    \$\begingroup\$ yes, this is a bug. My intention was to use these variables in the template. thanks! \$\endgroup\$ – Bruno Jun 1 '16 at 14:17
  • 3
    \$\begingroup\$ @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! \$\endgroup\$ – jpaugh Jun 1 '16 at 14:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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