On Tue, Dec 25, 2012 at 1:33 PM, Glyn Jackson <cfspa...@gmail.com> wrote:
> 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)) > > 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 -- You received this message because you are subscribed to the Google Groups "Django users" group. To post to this group, send email to django-users@googlegroups.com. To unsubscribe from this group, send email to django-users+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-users?hl=en.