cbv_fail.py

# If it's complex, Django's Generic CBVs will probably
# make your life harder. And if it's simple, they probably
# will too.

# Before:

from django.views import generic

class ReportPDFDetailView(generic.DetailView):
    model = DesignerReport

    def get_queryset(self):
        return self.request.user.designer.reports.filter(is_approved=True)

    def get(self, request, *args, **kwargs):
        self.object = self.get_object()
        report = self.object
        download_filename = '%s_%i-%i.pdf' % (report.designer.name, report.month_index, report.year)
        return internal_redirect_response(report.pdf_report, download_filename)


# After:

from django.shortcuts import get_object_or_404

def report_detail_view_pdf(request, pk=None):
    report = get_object_or_404(request.user.designer.reports.filter(is_approved=True),
                               pk=pk)
    download_filename = '%s_%i-%i.pdf' % (report.designer.name, report.month_index, report.year)
    return internal_redirect_response(report.pdf_report, download_filename)


# Notes:
#
# * Yes, the ```model = DesignerReport`` line in the ‘before’ is unnecessary, but that's 
#   the way it was written. The indirection of CBVs discourages comprehension and 
#   encourages cargo-culting.
#
# * In a similar way, ``self.object = self.get_object()``, is also unnecessary,
#   but it's clearly been added by someone who sensibly doesn't want to break
#   anything. You never know what other things a CBV is doing that you mustn't
#   break.
# 
# * And then, the `report = self.object` line, again unnecessary strictly
#   speaking, was clearly a helpful addition for the sake of clarity regarding
#   what type of object we're dealing with. Generic CBV methods are incredibly
#   unhelpfully named, because they are so generic.
#
# The rewritten version using a function can avoid all of this. In addition,
# it's massively simpler, clearer, and easier to reason about.

So if apply your notes to the example source code it will be

class ReportPDFDetailView(generic.DetailView):
    def get_queryset(self):
        return self.request.user.designer.reports.filter(is_approved=True)

    def get(self, request, *args, **kwargs):
        report = self.get_object()
        download_filename = '%s_%i-%i.pdf' % (report.designer.name, report.month_index, report.year)
        return internal_redirect_response(report.pdf_report, download_filename)

Instead of

def report_detail_view_pdf(request, pk=None):
    report = get_object_or_404(request.user.designer.reports.filter(is_approved=True),
                               pk=pk)
    download_filename = '%s_%i-%i.pdf' % (report.designer.name, report.month_index, report.year)
    return internal_redirect_response(report.pdf_report, download_filename)

6 lines vs 4 lines with the same functionality but with much cleaner source code which can be reused by extending and overriding any function in the View without touching base class. I think CBV is much better especially if you can write well structured OOP code. It is useless of course in case you are monkey-coder :-)

In your source code I want to extend the class by extracting the filename generation line into an extra function to be able generate public/secure download links without touching base PDF download function.

First, with regard to your rewrite - as I noted above, the additional, unnecessary lines were caused by the complex API of the CBV.

Second, the FBV is still shorter and much clearer than your rewritten CBV. I don't know why you would consider the CBV to be cleaner. In the name of "reuse", it involves a large stack of unnecessary base classes and ends up with more code! You can only consider this cleaner if you have a religious belief in "classes good, functions bad". For example, looking at the CBV version, what does it do in the case of the object not being found? With the FBV it is immediately obvious, with the CBV you have to have memorised what the base classes do, or go hunting.

Third - with regard to your additional requirements, of course for any code it's possible to come up with arbitrary, imaginary extra requirements that make one approach look better than the other. But you should cross those bridges when you come to them.

Fourth, we can in fact easily extend the FBV for your requirements, simply by adding a callback keyword argument, just like this:

def report_download_filename_generator(request, report):
    return '%s_%i-%i.pdf' % (report.designer.name, report.month_index, report.year)

def report_detail_view_pdf(request, pk=None, download_filename_generator=report_download_filename_generator):
    report = get_object_or_404(request.user.designer.reports.filter(is_approved=True),
                               pk=pk)
    return internal_redirect_response(report.pdf_report, download_filename_generator(request, report))

Further, this is better than the CBV approach in many ways, because the download_filename_generator parameter will be a standalone function which is more easily testable and properly separated than if you bundled it into a method on the class.

@spookylukey In my opinion, It's about the de-coupling of various parts of the flow for better extension and customisation at modular level of those parts. Ofcourse that comes with overhead of learning how it has been done so, but it's worth it when things start getting big and complex.

For ex.- In the above <10 line code, you may be tempted to think that FBV is shorter and better, but when the logic becomes complex and big (let's say ~50 lines), you'll thank the modular control CBVs provide every second of development.

Just my personal perception.