Django's CBVs were a mistake

Posted in:

I've written before about the somewhat doubtful advantages of Class-Based Views.

Since then, I've done more work as a maintenance programmer on a Django project, and I've been reminded that library and framework design must take into account the fact that not all developers are experts. Even if you only hire the best, no-one can be an expert straight away.

Thinking through things more from the perspective of a maintenance programmer, my doubts about CBVs have increased, to the point where I recently tweeted that CBVs were a mistake.

So I thought I'd explain my reasons here. First, I'll look at the motivation behind CBVs, how they are doing at solving what they are supposed to solve, and then analyse the problems with them in terms of the Zen of Python.

What problems do CBVs solve?

Customising generic views

People kept wanting more functionality and more keyword arguments to the list_detail views (and others, but that those especially, as I remember). The alternative was large copy-and-paste of the code, so people were understandably wanting to avoid that (and avoid writing any code themselves).

So, we replaced them with classes that allows people to override just the bit they need to override. This eliminates the need for code duplication, and removes the burden of lots of feature requests for generic views.

Or does it? Instead of tickets for keyword arguments to list_detail, it seems we have a bunch of other tickets asking for changes to CBVs, many of which can't be implemented as mixins or subclasses.

One of the problems is that if the view calls anything else (e.g. a paginator class, or a form class), you have to provide hooks for how it calls it, which means implementing methods that can be overridden. If you forget any, or if the thing you are calling gains some new keyword arguments, you've got feature requests, or duplication because someone had to override a larger method just to change one aspect of it. If you don't forget any, you've got dozens of little methods to document.

Also, there are problems like this attempt to mix FormView with ListView functionality. Fixing this will end up with similar amounts of copy-paste, but in this case it requires a fair bit of debugging first to realise you have a problem.

So I'm not convinced CBVs have made much difference here.

Eliminating flow control boilerplate

The classic example is editing using a form. You see this pattern again and again using function based views (FBVs from now on):

from django.shortcuts import render

def contact(request):
    if request.method == 'POST':
        form = ContactForm(request.POST)
        if form.is_valid():
            send_contact_message(form.cleaned_data['email'],
                                 form.cleaned_data['message'])
            return HttpResponseRedirect('/thanks/')
    else:
        form = ContactForm()

    return render(request, 'contact.html', {'form': form})

Without question this is tedious and annoying. CBVs reduce this to:

from django.views.generic.edit import ProcessFormView

class ContactView(ProcessFormView):
    form_class = ContactForm
    template_name = 'contact.html'
    success_url = '/thanks/'

    def form_valid(self, form):
        send_contact_message(form.cleaned_data['email'],
                             form.cleaned_data['message'])
        return super(ContactView, self).form_valid(form)

Much better!

However...

It's not really that much shorter. 8 lines compared to 11, ignoring imports. But now let's make it more realistic. We're going to have:

  • Initial arguments to the form that are based on the request object.

  • Priority users get a form with the option to indicate 'urgent' status to message, which results in a text message as well as an email. The template is also rendered a bit differently for them and needs a flag.

  • URLs defined using reverse as they should be.

FBV:

from django.core.urlresolvers import reverse
from django.shortcuts import render

def contact(request):
    high_priority_user = (not request.user.is_anonymous()
                          and request.user.get_profile().high_priority)
    form_class = HighPriorityContactForm if high_priority_user else ContactForm

    if request.method == 'POST':
        form = form_class(request.POST)
        if form.is_valid():
            email, message = form.cleaned_data['email'], form.cleaned_data['message']
            send_contact_message(email, message)
            if high_priority_user and form.cleaned_data['urgent']:
                 send_text_message(email, message)
            return HttpResponseRedirect(reverse('contact_thanks'))
    else:
        form = form_class(initial={'email': request.user.email}
                                  if not request.user.is_anonymous() else {})

    return render(request, 'contact.html', {'form': form,
                                            'high_priority_user': high_priority_user})

CBV:

from django.core.urlresolvers import reverse_lazy
from django.views.generic.edit import ProcessFormView

class ContactView(ProcessFormView):
    template_name = 'contact.html'
    success_url = reverse_lazy('contact_thanks')

    def dispatch(self, request, *args, **kwargs):
        self.high_priority_user = (not request.user.is_anonymous()
                                   and request.user.get_profile().high_priority)
        return super(ContactView, self).dispatch(request, *args, **kwargs)

    def get_form_class(self):
        return HighPriorityContactForm if self.high_priority_user else ContactForm

    def get_initial(self):
        initial = super(ContactView, self).get_initial()
        if not request.user.is_anonymous():
            initial['email'] = request.user.email
        return initial

    def form_valid(self, form):
        email, message = form.cleaned_data['email'], form.cleaned_data['message']
        send_contact_message(email, message)
        if self.high_priority_user and form.cleaned_data['urgent']:
             send_text_message(email, message)
        return super(ContactView, self).form_valid(form)

    def get_context_data(self, **kwargs):
        context = super(ContactView, self).get_context_data(**kwargs)
        context['high_priority_user'] = self.high_priority_user
        return context

(A few lines could be shaved by not using super() in a number of places, but only at the expense of future confusion/problems if a maintainer was expecting the normal declarative behaviour.)

Notice:

  1. I really want high_priority_user to be a local variable that is calculated once, and used in a couple of places. With a function, that's what I have. With a CBV, I have to simulate it using an attribute on self. I also need to override the dispatch() method just to create it. These are both ugly hacks.

  2. The CBV version is extremely noisy, due to all the calls to super(). This would be improved in Python 3 (at the expense of a rather magical super() builtin), but is still far from perfect. Every time you override a method, you have to mention it twice.

  3. The CBV version is now significantly longer - 24 non-blank lines compared to 17. Since I'm using the same APIs for requests and forms, and doing the same thing, this can only mean that the amount of boilerplate has significantly increased. Sure, I've removed the flow control boilerplate, but must have added some other type.

  4. Even if you know the Form API very well, you are going to have to look up the docs or the source code to find get_initial() etc. and ensure you get the signature correct. You have to know two sets of APIs to use a form, instead of one.

  5. In the CBV, flow control is totally hidden. In the process of abstracting away the duplication, we've also hidden the order of execution. I've ordered my methods in the order they are called (I think), but that may or may not be obvious to anyone else, and nothing forced me to do it.

  6. Because of the last point, it is massively more difficult to debug. Which of the two would you rather maintain? Which do you think a maintenance programmer, who has never seen this code before, would rather maintain?

    To understand what is going on, you've got an intimidating stack of base classes to navigate, compared to a single function.

The fundamental problem here is that Python sucks at implementing custom flow control. Not many languages shine here. Ruby has blocks, which help. Haskell has a pretty good story due to a combination of succinct function definition, lazy evaluation and the way that IO works. Lisp has macros. But with Python, we are limited to: abusing generators, abusing the with statement, or classes and the template method design pattern (which is basically what CBVs use).

I think we decided to use the latter because it's one of the only options we've got, but failed to notice that it's just not very good.

There are worse things that can happen with shifting requirements. What if you want two different forms on the same page? I've done this on more than one occasion, and it can be a useful thing – when you present the user with two different courses of action, and you've got completely different info they need to fill in.

It would be a nightmare to get CreateView or UpdateView to do this. You will either produce a monstrosity, or you'll have to start from scratch with an FBV. You've been seduced down a tempting, calm stretch of water, and then left high and dry when you come to the end of what CBVs offer. If you start with an FBV, it's an easy change.

Overall, when I work on CBVs, even views I've created, I start to feel like I'm working on a classic ASP.NET Page class. It is bringing back painful memories! We’re not as bad as that yet, but methods that simply modify some data on self are a code-smell that we are heading in that direction.

Another way of looking at it is that with CBVs, views have become an instance of the ‘framework pattern’ instead of the ‘library pattern’. With a framework, your application code gets called by the framework code, and you can easily end up having to understand how the framework is implemented.

With a library, the library code gets called by your application code, and this is in general much more flexible, much easier to document and much easier to debug. We ought to be moving Django more in the direction of a library.

With the comparison to ASP.NET, I'm also basically saying CBVs are not Pythonic. I'll back up this claim in terms of selected parts of the Zen of Python.

Are CBVs Pythonic?

Beautiful is better than ugly

This is a subjective one, but the noise of all the super() calls, get_context_data() compared to a simple {} etc. is ugly to me.

Simple is better than complex

Let's notice, first off, that FBVs are simpler than CBVs, according to the basic meaning of having fewer parts.

A class is a datastructure with attributes and functions attached. A function is just a function. So a class is more complex than a function. If we ever use a class where a function would work, we have to justify the additional complexity.

Complex is better than complicated

Are CBVs complicated? Just read the Django source if you think they are not.

In one app I wrote, I had a mixin that implemented a bit of common flow control for forms – namely it returned a JSON response with validation errors for certain types of requests. This meant I didn't need separate views for the AJAX validation, and with CBVs I eliminated all the boilerplate. Great!

Well, it was, until I found a crazy problem to do with the order in which different base classes set things up. To solve my problem, I ended up writing this:

# MRO problem: we need BaseCreateView.post to come first in MRO, to
# provide self.object = None, then AjaxyFormMixin must be called before
# ProcessFormView, so that the right thing happens for AJAX.

class AjaxMroFixer(type):

    def mro(cls):
        classes = type.mro(cls)
        # Move AjaxyFormMixin to one before last that has a 'post' defined.
        new_list = [c for c in classes if c is not AjaxyFormMixin]
        have_post = [c for c in new_list if 'post' in c.__dict__]
        last = have_post[-1]
        new_list.insert(new_list.index(last), AjaxyFormMixin)
        return new_list

It's a metaclass, and implements the mro() method that allows you to override the method order resolution. I am not proud of this code. (For those who don't get English understatement, please understand what I'm saying: this code is horrific). Before writing this code, I didn't know that the mro() method existed. Although I value the education, if a framework forces you to learn about type.mro() and implement it, it is doing something wrong.

Flat is better than nested

The hierarchy of classes is certainly a form of nesting, whereas functions are flat. I think CBVs might be considerably better if you started by writing your own, so that you had base classes that did everything you needed, but nothing more, and ended up with a very flat hierarchy.

Readability counts

Get some people who don't know Django to read ContactView above and figure out what it does, and some others to read the function version, and see who has the easier time. Ask them what happens, for example, when the form is invalid. There is no contest here.

Explicit is better than implicit

With a CBV, by inheriting from a class you are inheriting all the behaviour of that class, and all its parent classes. You could argue this is explicit, since you've explicitly indicated the base class, but you haven't indicated all the parent classes – they come automatically. So it's more like an implicit request in practice.

Of course, this is always true with OOP to some extent – you are inheriting a bunch of behaviour precisely because you don't want to define it again. But let's notice that it does have its downsides.

For example, let's play spot the difference between the following two views – an FBV:

from django.shortcuts import render

def my_view(request):
    return render(request, "my_template.html", {})

and a CBV:

from django.views.generic import TemplateView

class MyView(TemplateView):
    template_name = "my_template.html"

Can you see the important difference? Try to answer before reading on.

I'm talking only about the functional differences when this view is accessed by a client, not differences of code organisation or re-use or performance.

Well, TemplateView inherits from View which provides a dispatch() method, and TemplateView provides a get() method which will handle all GET (and HEAD) requests, by the logic defined in View.dispatch(). However, neither defines a post() method, which means that you will get a "405 Method Not Allowed" error if you try POST or other HTTP verbs, to the CBV view, whereas you will get a 200 with the FBV.

In the CBV, all of this logic has been invoked implicitly. Nothing in what I wrote was an explicit request for 405s for POST requests, but I got it because I inherited from TemplateView – even though using a template does not imply that behaviour.

(By the way, this issue caused a real problem in a site I wrote, which was easily debugged because I knew a lot about the internals of CBVs, and therefore easily fixed, but it still adds noise to my class – code that can only be explained by reference to some inherited behaviour I didn't really want).

Of course, as already mentioned, you can say the same thing whenever you inherit from a class. But, in my opinion, it is one of the disadvantages of OOP, and it is showing itself here. The question is: do the advantages of inherited behaviour outweigh the disadvantages of implicit behaviour?

There should be one way to do it

I already mentioned one case where the CBV method is just not going to work for you, or is not going to be worth it – needing two different forms on the same page. But in the real views I write, I suspect probably the majority would not fit easily into a CBV.

So, in your project you now need both FBVs and CBVs – you've got two ways to do it. When a maintenance programmer comes along, and needs to do some similar work that involves a form, they will be confused. Which pattern do they start from?

The simple way to avoid this is to avoid the less flexible solution – avoid using CBVs.

So simply having two different ways of building up views is a violation of this principle, but within CBVs there are also instances.

First, there are also problems with the attempt to use declarative style, which is perhaps the most attractive feature of the CBV API. So, you need initial arguments to your form? Just define the initial attribute on your class. Unless, however, you need it to be dynamic – you'll have to define get_initial() instead.

Also, it often isn't obvious where to add certain bits of code. There is often more than one choice of method to override when you come to add a little bit of functionality e.g. get() or dispatch(), and which you choose sometimes has subtle implications, and sometimes doesn't matter. With FBVs, two different Django developers tasked with the same modification to an existing view would often produce identical or nearly identical patches, but I suspect that would be rarer with CBVs.

UPDATE 2015-12-31:

The mantra of "There should be one way to do it" means that very often CBVs will be used when they are really inappropriate, once you've started down that path. That explains silliness like the following:

class FeedbackView(SingleObjectMixin, generic.TemplateView):
    template_name = 'dashboard/designers/feedback.html'
    model = DesignerFeedback
    context_object_name = 'feedback'
    key = "feedback"

    def get_object(self, queryset=None):
        self.object = get_object_or_404(
            DesignerFeedback,
            pk=self.kwargs['pk'],
            designer=self.request.user.designer,
        )
        return self.object

    def dispatch(self, request, *args, **kwargs):
        self.object = self.get_object()
        return super(FeedbackView, self).dispatch(request, *args, **kwargs)

That was a real example taken from a project I'm working on, and from the perspective of a maintenance programmer it's a nightmare, involving multiple levels of indirection to achieve a very simple task. It should have been written as this very simple code which is less than half the size:

def view_feedback(request, pk=None):
    feedback = get_object_or_404(
        DesignerFeedback,
        pk=pk,
        designer=request.user.designer,
    )
    return render(request, 'dashboard/designers/feedback.html',
                  {'feedback': feedback})

It was probably written as the much more complicated version due to either cargo-culting (which becomes necessary when you have complicated class hierarchies that no-one really understands) or deliberate consistency with the surrounding code.

If the implementation is hard to explain, it's a bad idea

Looking at the implementation, you'll find things like MultipleObjectTemplateResponseMixin, as well as MultipleObjectMixin and TemplateResponseMixin, and the former is not just the composition of the other two. This is just one of many signs that things are going wrong. If you can really compose behaviour just by adding mixins, MultipleObjectTemplateResponseMixin should not be needed. Explaining things like this is hard, and the reason is that you just can't build up complex views using classes and mixins.

Conclusion

Overall, I think CBVs make:

  • very simple views slightly shorter, much cleaner (and significantly harder to debug, but you don't need to debug them because they are simple);

  • views of medium complexity significantly longer, with more boilerplate and noise and much harder to debug;

  • views of high complexity almost impossible.

You only gain for the simple views, but they were simple anyway, just slightly tedious. Is this advantage really enough to outweigh the disadvantages I've listed?

To be honest, I regret dropping the function based generic views for CBVs. There was an alternative solution to the tickets asking for them to do more: WONTFIX.

Generic views were simple shortcuts for common problems. They didn't actually involve very much code, and if you needed to duplicate some of it you weren't duplicating much. We should have just said: this is what generic views do, if you need them to do something else then write your own. Because that is what you have to say with class based generic views anyway, just slightly later.

Regarding going forward, I say: stop the rot.

If you're starting a new project, I recommend avoiding CBVs, or just wrapping them in thin functional wrappers, like this one for ListView (or, with django-pagination a simple 3-line view function is probably all you need and is actually easier than subclassing ListView).

For Django core, let's fix up the main problems and bugs CBVs have, and then leave them as solutions to simple problems.

But please: let's not move everything in Django-world – whether Django core/contrib or resuable apps – to CBVs. Just use a function, and stop writing classes.


Update: 2013-03-09

My opinions have changed slightly since I wrote the above, due to comments below and other helpful blog posts. I'd summarise by saying:

  • There are some places with CBVs really shine, especially when you are writing many similar simple views.

  • You can avoid some of the problems I mentioned by using your own class hierarchy, that you control completely, and making it flat, and specific to your needs.

  • I still prefer to write function based views. I've spent too many hours debugging class hierarchies of different kinds, and seen too many view functions that started out fitting into the kind of patterns that CBVs provide, and then breaking them in big ways.

Comments §

Comments should load when you scroll to here...