Bad Django Code

Posted on November 29, 2009 at 17:41

I have been working with Django for a solid 4 months now and learned a lot during that period. I read a lot about how to make sure your code isn't bad. So far this is what I've learned.

These are all codes that are bad in most cases, but there can be specific ways they can be useful.

Context Processors

def my_context(request):
    """
    Puts a mess of things in my context
    """

    return {
        'ads': Ad.objects.filter(live=True),
        'user_profile': request.user.profile,
        'store_url': settings.STOREURL,
    }

This is a context processor with some problems, particularly we're doing database calls. Context processors will get invoked every single time you touch a template. For example you have a base template which is used by an other template which has 5 includes, now you are going to call your context processor at every level, which is 7 times. You really want to use context processors as least as possible. When you really need to, you will have to make it as efficient as possible because it is going to be called a lot.

The only thing where context processors should ever be used for is in for example 'store_url': settings.STOREURL,. They also could be useful in middleware, caching things or when applying it in a custom template tag.

Models

class BaseObject(models.Model):
    name = models.CharField(max_length=255)

class Student(BaseObject):
    other_field = models.Charfield(max_lenght=50)
    # ...

class Teacher(BaseObject):
    other_field = models.Charfield(max_lenght=50)
    # ...

class Staff(BaseObject):
    other_field = models.Charfield(max_lenght=50)
    # ...

class Volunteer(BaseObject):
    other_field = models.Charfield(max_lenght=50)
    # ...

This is a bad example of using models, you could call this example the "model to rule them all". At first sight this looks like a good code. This is because it is a common pattern in object orientated design, you have a base subject with some common attributes and than some child objects which inherit from that.

In Django this is bad, when you inherit a model you are going to do what is called multi-table inheriting. This means that all the attributes of your base subject will be in one table and than all the child objects will have their own tables with a foreign key up to the parent and will automatically do a join when you do queries on them. The problem with this it will horribly slow down your database and is an anti-pattern of relational database designs.

For object orientated designs this looks like a great design, but it is meant for a relational database which makes the design unusable.

Iterating Query Sets

for user in User.objects.all():
    print user
    # Do a few other things to the user

In this example you are going to issue a separate query for each object in User. The way to get around this would be if you cast the query set as a list, it will get all your data back in one query.

An other thing you should look out for is the .all(), most of the time you will get more data back than you will need.

Templates

class MyModel(models.Model):
    count = models.IntegerField(default=0)

    def increment(self):
        self.count += 1
        self.save()

{% block content %}
    {{ my_model.increment }}
{% endblock %}

Here is something to look out for in your templates. We have a model and it has a method that changes the data of the model and in our template we're calling that method. Django does a pretty good job of protecting you from putting logic into your templates but it is still possible as shown here. It only will happen if you have a method that doesn't expect any parameters.

Views

def show_post(request, slug):
    post = get_object_or_404(BlogPost, slug = slug)

    return render_to_response('post.html', {
        'post': post,
    }, RequestContext(request))

def show_post(request, slug):
    post = get_object_or_404(Video, slug = slug)

    return render_to_response('video.html', {
        'video': video,
    }, RequestContext(request))

def show_post(request, slug):
    post = get_object_or_404(Book, slug = slug)

    return render_to_response('book.html', {
        'book': book,
    }, RequestContext(request))

If you have a whole lot of views that are doing virtually the same thing, you're probably doing something wrong. In this specific case, if you have views that are just getting something and putting it into a template you're doing it wrong. There are generic views that are already written and do the same thing and will have less bugs than you probably have.


Add your comment:

What's your name?

You can use Markdown.

You're going to say: