Code guidance please review.

[Glyn Jackson]

Hi,

I'm new to Django and just need some guidance.

Below is my attempt at creating a simple app that displays all transactions and lists the grand total called 'points' for a particular user. Now the following works and does just that, however, I'm not 100% if this is the 'correct' way nor really what a 'manager' is.

Could someone look at the following code and advice?

Many, many thanks O, and Happy Christmas everyone!


models.py

from django.db import models
from django.contrib.auth.models import User




class TransactionManager(models.Manager):

    def Transaction_list(self,thisUser):
        list = Transaction.objects.filter(user=thisUser)
        return list

    def points_total(self,thisUser):
        return  Transaction.objects.filter(user=thisUser).aggregate(total_points=models.Sum('points'))



class Transaction (models.Model):

    statusOptions = (
        (0, 'Pending'),
        (1, 'Added'),
        (2, 'Deducted'),
        (3, 'Processing'),
        )

    user = models.ForeignKey(User)
    points = models.IntegerField(verbose_name=("Points"), default=0)
    created = models.DateTimeField(("Created at"), auto_now_add=True)
    updated = models.DateTimeField(verbose_name=("Updated at"), auto_now=True)
    status = models.IntegerField(default=0, choices=statusOptions)

    objects = TransactionManager()


views.py

from django.template import RequestContext
from django.contrib.auth.decorators import login_required
from django.shortcuts import render_to_response
from points.models import Transaction

@login_required
def history(request):

    thisPoints = Transaction.objects.Transaction_list(request.user)
    totalPoints = Transaction.objects.points_total(request.user)
    context = {'points':thisPoints,'totalPoints':totalPoints}
    return render_to_response('points/history.html', context, context_instance=RequestContext(request))

[Bill Freeman]

This is certainly reasonable over all.

I would point out, however, that manager instance, "self" in your Transaction_list and points_total methods, *is* Transaction.object, so (assuming that you keep them as manager methods), you could just use "self.filter...".  Further, in the interests of not having duplicate code to maintain, you could implement points_total using "self.Transaction_list(thisUser).aggregate...".  A style point, while it is customary to use an initial upper case letter on model and manager (and other class) names, method names, especially those using underscores as word separators, are usually all lower case.  But, too, since it is part of a TransactionManager, it is a bit redundant to name the method Transaction_list.  You could call it just "list", but perhaps "for_user" is clearer, allowing you to say (in the view) "Transaction.objects.for_user(request.user)", which is nearly the simple English for what you are doing.

It is possible, but not conventional, to create these methods as classmethods on the Transaction class, rather than requiring the implementation of a custom manager class, e.g.:

    @classmethod
    def for_user(cls, thisUser):
        return cls.objects.filter(user=thisUser)

You would use this in the view like so:

    thisPoints = Transaction.for_user(request.user)

There is a mild preference for the manager approach, since it doesn't pollute the model name space.  A model already has an "objects" attribute, so accessing the manager doesn't require a new attribute.  Using the classmethod means that there is one more attribute on a model instance, which should largely be about the instance (table row), rather than about accessing instances.  Not, however, a big deal.

But both of these methods are so simple that hand coding them in the view has appeal.  It makes the code local, rather than forcing the reader to find the manager code to know for sure what's happening.  If the function were more complex, or if it is used in more than one place, then the separate manager methods are to be preferred.

And note that you can use a queryset multiple times without having to recreate it.

    @login_required
    def history(request):
        thisPoints = Transaction.objects.filter(user=request.user)
        totalPoints = thisPoints.aggregate(total_points=models.Sum('points'))
        ...

The use of "thisPoints" in calculating "totalPoints" doesn't change "thisPoints".  You can still enumerate it in the template, and get the same result.

Bill

[Glyn Jackson]

ke1g, thank you for such a well written reply, clearly explained, I learn a lot. again thanks.

The only bit I'm struggling to understand is the last part you state

"The use of "thisPoints" in calculating "totalPoints" doesn't change "thisPoints".  You can still enumerate it in the template, and get the same result."

[Bill Freeman]

Manager and queryset methods like "filter" return a new queryset, which is a python class instance holding all the things that you have said about your query, but it hasn't touched the database as yet.  When you use something like "filter" on an existing queryset, the original information is still in the original queryset, it has been copied as necessary in the construction of the new queryset.

A queryset is "evaluated" only when you try to use what's in the queryset, such as "list(qs)" or "for r in qs" (including the template language's "for"), or when you print it in the manage.py shell, as examples.  And only when it is evaluated, does it wind up composing SQL, send it to the database, parse the result into model instances (and cacheing these as part of doing requests with large numbers of results in chunks, among other things).  You really get quite a number of splendid optimizations when you use the ORM.

So at the time I showed applying "aggregate" to the "thisPoints" queryset, "thisPoints" hadn't touched the database.  At that time, it is specifically NOT a list of instances.  It is still a queryset class instance.  And the "aggregate" method uses what's in it, but doesn't change it.  Later, in your template, where you presumably apply a "for" template tag to it, is when it uses what it knows to get stuff for you from the database.

Bill

[Glyn Jackson]

thank you again Bill :0 makes perfect sense now.