Django's new class based views are very cool. I am starting to clean up an existing project using them, and lots of existing views are turning into declarative code. But it makes me worried about the ravioli effect.
Take my feedback form as an example. It is a simple view, and typical of code in my project — a couple of calls to some standard utilities. One of the them, standard_extra_context, adds some standard context variables depending on what it passed in (like title, description etc.) which are used in the HTML metadata and in the page itself. Another, json_validation_request allows a form view to be re-used for AJAX validation. The form declaration and view look like this:
class FeedbackForm(CciwFormMixin, forms.Form):
email = forms.EmailField(label="Email address", max_length=320)
name = forms.CharField(label="Name", max_length=200, required=False)
message = forms.CharField(label="Message", widget=forms.Textarea)
def feedback(request):
c = standard_extra_context(title=u"Contact us")
if request.method == 'POST':
form = FeedbackForm(request.POST)
json = utils.json_validation_request(request, form)
if json: return json
if form.is_valid():
send_feedback(form.cleaned_data['email'],
form.cleaned_data['name'],
form.cleaned_data['message'])
c['message'] = u"Thank you, your message has been sent."
else:
form = FeedbackForm()
c['form'] = form
return shortcuts.render_to_response('cciw/feedback.html',
context_instance=template.RequestContext(request, c))
It does one bad thing, namely it doesn't redirect on success, but apart from that it is fine. But there is this annoying boilerplate of flow control. So, very quickly, after creating some standard mixins and base classes that are equivalent to my standard utilities, I now have the following (which now does the redirect-on-success):
class FeedbackForm(CciwFormMixin, forms.Form):
email = forms.EmailField(label="Email address", max_length=320)
name = forms.CharField(label="Name", max_length=200, required=False)
message = forms.CharField(label="Message", widget=forms.Textarea)
class FeedbackBase(DefaultMetaData):
metadata_title = u"Contact us"
class FeedbackFormView(FeedbackBase, AjaxyFormView):
form_class = FeedbackForm
template_name = 'cciw/feedback.html'
def get_success_url(self):
return reverse('cciwmain.misc.feedback_done')
def form_valid(self, form):
send_feedback(form.cleaned_data['email'],
form.cleaned_data['name'],
form.cleaned_data['message'])
return super(FeedbackFormView, self).form_valid(form)
class FeedbackDone(FeedbackBase, TemplateView):
template_name = 'cciw/feedback_done.html'
feedback = FeedbackFormView.as_view()
feedback_done = FeedbackDone.as_view()
(DefaultMetaData is a TemplateResponseMixin subclass that replaces standard_extra_context, FeedbackBase defines common attributes for FeedbackFormView and FeedbackDone. AjaxyFormView is a FormView subclass and replaces utils.json_validation_request. The last two lines are simply there for the sake of not adding imports to my urls.py).
Now, this is a big improvement in one way. All standard flow control has been removed, so it is very DRY in that sense, and the details shown are the details which are relevant to the one task of handling a feedback form. The way to read this view is as declarative code, similar to how you read models and forms:
FeedbackFormView:
- is a feedback view
- so it has the title 'Contact us'
- and other default metadata
- so it has the title 'Contact us'
- is a form processing view (which has been ajaxified)
- has
FeedbackFormas the form - has
"cciw/feedback.html"as the template. - has the 'success url' equal to the 'feedback_done' URL.
- does the following with a valid form:
- sends a feedback message.
Only the last of these is really imperative, and this is quite a nice way to think about a chunk of code.
However, suppose we have a maintenance programmer who needs, let's say, to make the template display a certain message on the basis of a certain URL parameter.
In the first case, it is pretty obvious what to do — a couple of lines inserted before the render_to_response call will work fine, with a corresponding change to the template.
In the second, it is also fairly easy to do — you would just have to define an appropriate get_context_data() method. But to find that out, you've got a fairly intimidating stack of mixins and base classes to investigate. In fact, it's more like a tree not a stack (though I think I may already have some diamond inheritance in there by accident).
This is classic ravioli code. As one person on that c2 page says, “this style maximizes maintainability by old hands at the expense of comprehensibility by newcomer”.
Put it this way: suppose you want to go from the current version to the previous version, in a precise and controlled way. You could trace through all the functions that would be called, starting with `as_view()` and working through till you collected all the code paths that were being used, but it would take you a lot longer than the forward transformation just took me. And this is exactly the transformation you might need to do if, for example, you needed to make a fundamental change to the flow control while keeping most of the code intact. It is faster to start from scratch with the declarative reading above — but that will only work if the view really is declarative, and there are no sneaky surprises in the base classes, and even then it may take some digging.
So, what am I saying? Don't do this? I guess I'm just saying “be aware of this trade-off”. As Shannon Behrens recently blogged, there are times when fixing DRY violations are not worth it.
Comments §
Surely this is spaghetti, tagliatelle or
even papardelle code?
;)
The result end up being quite difficult to understand to new comers and myself. The maintenance and debugging suffered too because of the deep hierarchy that followed.
Be careful with Class based view. Use them with moderation and keep the inheritance simple.
@Ziming: I too found it much easier to browse the source than to read the docs for this. I don't think it is because of poor documentation, either. OO interfaces are just much harder to describe than functional ones.
I meant to point out that another way to use the new class-based views is by composition and wrapping rather than inheritance - i.e. use the result of
SomeView.as_view()as a simple view function. This allows you to convert the OO interface into a simple functional one.However I still feel the documentation could better benefit from giving us a trace of the more important methods that are called from the first to the last.
Also, you forgot to mention that the new approach appears to be just about the same number of lines as the old one. :)
Another way to achieve a more declarative approach to the standard page things could be deriving a class for that, only, then use it in your functional view.
I do something a bit like that for most complex sites I've built. Instead of using template inheritance and having to stuff a lot of things into contexts and template tags, most of my views simple render a <div> with their content, then end with a return render_page(page, divcontent) which takes the page configuration (title, menu, etc.) and the <div> produced by the view and constructs the final complete HTML page.