Redundant code in Django models.py. How do I improve it?

Question

I am trying to create a task list with each task having a datetime attribute. The tasks needs to be in order with t_created being the first and t_paid being last. The order is shown in step_datetime. The description for each tasks is in STEPS.

I currently have two methods all_steps and next_step that shows the task list information. The two methods also need to display the name of the user_created, but that variable won't be defined until the methods are called. That's why I am doing a string replace method.

I feel like I am repeating my code a lot, and I want to follow the DRY principle of Django. Is there any way I could improve this code?

Here is my full code:

class Order( models.Model ) :
    def __unicode__( self ) :
        return unicode( self.id )

    def comments_count( self ) :
        return OrderComment.objects.filter( order = self.id ).count()

    def all_steps( self ) :
        user = self.user_created.first_name

        steps = []
        step_datetime = [
            self.t_created,
            self.t_action,
            self.t_followup_one,
            self.t_vendor_appt_one,
            self.t_vendor_appt_two,
            self.t_work_done,
            self.t_followup_two,
            self.t_paid,
        ]

        for ( i, step ) in enumerate( self.STEPS ) :
            steps.append( ( step_datetime[ i ], step.replace( '<user_created>', user ), ) )

        return steps

    def next_step( self ) :
        user = self.user_created.first_name

        step = 0
        if self.t_action is None :
            step = 0
        elif self.t_followup_one is None :
            step = 1
        elif self.t_vendor_appt_one is None :
            step = 2
        elif self.t_vendor_appt_two is None :
            step = 3
        elif self.t_work_done is None :
            step = 4
        elif self.t_followup_two is None :
            step = 5
        elif self.paid is None :
            step = 6

        return str( step ) + ": " + self.STEPS[ step ].replace( '<user_created>', user )


    STEPS = [
        "Review, then either approve or reject the order.",
        "Follow up with <user_created>",
        "Contact the vendor to get a quote and arrange an appointment for <user_created>.",
        "Review the quote, (get owner approval), then arrange a second appointment for the repairs.",
        "Confirm the finished repairs and pay the vendor.",
        "Follow up again with <user_created>",
        "Confirm payment and close the order.",
    ]

    ACTION_CHOICES = (
        ( 'p', 'pending'  ),
        ( 'a', 'approved' ),
        ( 'r', 'rejected' ),
        ( 'c', 'closed'   ),
    )

    user_created      = models.ForeignKey( User, related_name = 'user_created', verbose_name = 'created by' )
    user_action       = models.ForeignKey( User, related_name = 'user_status' , verbose_name = 'action by' , null = True, blank = True )
    t_created         = models.DateTimeField( auto_now_add = True, verbose_name = 'created' )
    t_action          = models.DateTimeField( null = True, blank = True, verbose_name = 'action'             )
    t_followup_one    = models.DateTimeField( null = True, blank = True, verbose_name = 'first follow-up'    )
    t_vendor_appt_one = models.DateTimeField( null = True, blank = True, verbose_name = 'first appointment'  )
    t_vendor_appt_two = models.DateTimeField( null = True, blank = True, verbose_name = 'second appointment' )
    t_work_done       = models.DateTimeField( null = True, blank = True, verbose_name = 'work done'          )
    t_followup_two    = models.DateTimeField( null = True, blank = True, verbose_name = 'second follow-up'   )
    t_paid            = models.DateTimeField( null = True, blank = True, verbose_name = 'paid'               )
    action            = models.CharField( max_length = 1, choices = ACTION_CHOICES, default = 'p' )
    quote             = models.DecimalField( max_digits = 8, decimal_places = 2, null = True, blank = True )
    payment           = models.DecimalField( max_digits = 8, decimal_places = 2, null = True, blank = True )
    items             = models.ManyToManyField( Item, null = True, blank = True )
    t_modified        = models.DateTimeField( auto_now = True, verbose_name = 'modified' )

After accepting @Dougal's answer. I changed some of the variables around and came up with this:

def all_steps( self ) :
    user = self.user_created.first_name

    return [
        ( getattr( self, attr ), task.format( user = user ) )
        for ( attr, task ) in self.TASKS
    ]

def next_step( self ) :
    user = self.user_created.first_name

    task_num = next(
        ( i for ( i, ( attr, task ) ) in enumerate( self.TASKS ) if getattr( self, attr ) is None ),
        None
    )

    if task_num == None :
        return "Done!"
    else:
        return "{number}: {task}".format(
            number = str( task_num + 1 ),
            task   = self.TASKS[ task_num ][ 1 ].format( user = user )
        )

TASKS = (
    ( "t_action"         , "Review, then either approve or reject the order." ),
    ( "t_followup_one"   , "Follow up with {user}." ),
    ( "t_vendor_appt_one", "Contact the vendor to get a quote and arrange an appointment for {user}." ),
    ( "t_vendor_appt_two", "Review the quote, (get owner approval), then arrange a second appointment for the repairs." ),
    ( "t_work_done"      , "Confirm the finished repairs and pay the vendor." ),
    ( "t_followup_two"   , "Follow up again with {user}." ),
    ( "t_paid"           , "Confirm payment and close the order." ),
)

score 3 · Answer 1 · answered Mar 08 '12 at 18:17

3

You can do things like:

for prop in ("t_created", "t_created2" ... ):
    val = getattr(self, prop)
    # some logic that works with that, maybe uses setattr

answered Mar 08 '12 at 18:17

Marcin

48,559
18
128
201

1

Ok the magic function that I didn't know about was `getattr`. Thanks! – hobbes3 Mar 08 '12 at 18:35

score 2 · Accepted Answer · edited May 23 '17 at 12:29

2

Adding on to @Marcin's answer:

You could make a tuple of the property names (say _STEP_NAMES at the module level; you could also make it at the class level, like STEPS, or even just combine the two into a tuple of pairs of attributes and names; that might be a little cleaner). Also, STEPS should probably be a tuple, since it shouldn't be modifiable at runtime.

Then you can reduce your code to:

def all_steps(self):
    user = self.user_created.first_name
    return [(getattr(self, attr), step.replace('<user_created>', user))
            for attr, step in zip(_STEP_NAMES, self.STEPS)]

def next_step(self):
    user = self.user_created.first_name
    step = next((i for i, attr in enumerate(_STEP_NAMES)
                        if getattr(self, attr) is None),
                None) # assumes Python 2.6+
    if step == None:
         return "Done!"
    else:
         return str(step) + ": " + self.STEPS[step].replace('<user_created>', user)

If you need Python 2.4/2.5 compatability, the next line can be replaced by

try:
    step = (i for i, attr in enumerate(_STEP_NAMES) if getattr(self, attr) is None).next()
except StopIteration:
    return "Done!"
return str(step) + ": " + self.STEPS[step].replace('<user_created>', user)

edited May 23 '17 at 12:29

Community

1
1

answered Mar 08 '12 at 18:32

Danica

28,423
6
90
122

So what I did with `.replace( '', user )` is the best (or only) method? – hobbes3 Mar 08 '12 at 18:37
1

@hobbes3 It's certainly not the only method, but I don't really know a better one. One alternative might be to replace the elements of STEPS with e.g. `lambda s: "Follow up with %s." % s` and then do `self.STEPS[step](user)`; that's not really any better IMO, though it is a little more flexible. That would require static descriptions to be a `lambda` also. You could also just use `%(user_created)s` in the original string in STEPS and then use `self.STEPS[step] % {'user_created': user}`; that's probably a little nicer than the `.replace` approach, I think. – Danica Mar 08 '12 at 18:40
How does `[ (x, y) for n in list ]` work? It does `(x, y)` for each iteration of the for loop? – hobbes3 Mar 08 '12 at 21:11
Actually I don't know how your `next( i for ..., None )` works either. Could you care to explain? Thanks. – hobbes3 Mar 08 '12 at 21:19
1

@hobbes3 The one with brackets is a [list comprehension](http://docs.python.org/tutorial/datastructures.html); the `next` thing is a [generator expression](http://docs.python.org/release/2.5.2/ref/genexpr.html), which is similar but only actually evaluates as needed. The `next` function just gets the first element out, or returns `None` if there aren't any. Here's [some more on generators](http://stackoverflow.com/questions/1756096/understanding-generators-in-python). – Danica Mar 09 '12 at 01:13

Redundant code in Django models.py. How do I improve it?

2 Answers2