Django signals > right approach?

Question

In my signals.py I am doing the import for the function unique_order_reference_generator within the function reserveditem_create_order_reference. I wonder if that is the right approach, or is there a better solution?

file utils.py

from lumis.utils import get_random_string

from .models import Order, ReservedItem


def unique_order_reference_generator():

    new_id = get_random_string(length=10)

    reserved_item = ReservedItem.objects.filter(
        order_reference=new_id
    ).exists()
    order = Order.objects.filter(order_reference=new_id).exists()

    if reserved_item or order:
        return unique_order_reference_generator()  # TODO Marc: Test
    else:
        return new_id

file signals.py

# Generate order reference
def reserveditem_create_order_reference(sender, instance, **kwargs):
    from .utils import unique_order_reference_generator

    if not instance.order_reference:
        instance.order_reference = unique_order_reference_generator()

file app.py

class OrdersConfig(AppConfig):
    name = 'orders'

    def ready(self):

        #Pre save signal for ReservedItem model
        reserved_model = self.get_model('ReservedItem')
        pre_save.connect(
            reserveditem_create_order_reference,
            sender=reserved_model,
            dispatch_uid="my_unique_identifier"
        )

Is it working for you? Are there any problems, performance issues or something else? — Ralf, Jun 15 '18 at 20:27
The other option would be to override the `ReservedItem.save()` method to assign the random `order_reference` there. — Ralf, Jun 15 '18 at 20:28
Hint: you could also change the `unique_order_reference_generator` function to use a while loop instead of recursion if you want to cut down on unnecessary overhead. — Ralf, Jun 15 '18 at 20:30
Hi Ralf, I think the .save method is a very good idea. Further, I am struggling to understand how to use the while loop will help to cut down the function. In the while loop, I would still have to check the order_reference already exists? — , Jun 15 '18 at 22:45

Ralf · Accepted Answer · 2018-06-19T11:09:23.180

In the comments to the question I mentioned, as a side note, that you could refactor the function to avoid recursion.

This is not too important to the question on hand, but it could improve performance a little bit:

utils.py

from lumis.utils import get_random_string
from .models import Order, ReservedItem

def unique_order_reference_generator():
    # initial values
    reserved_item_exists = True
    order_exists = True

    while reserved_item_exists or order_exists:
        new_id = get_random_string(length=10)

        reserved_item_exists = ReservedItem.objects.filter(
            order_reference=new_id).exists()

        if not reserved_item_exists:
            # check 'Order' model only if there is no 'ReservedItem', because
            # why do a search in 'Order' when we already know that this
            # 'new_id' is taken
            order_exists = Order.objects.filter(
                order_reference=new_id).exists()

    return new_id

Recursion helps to make the code easier to read, but it almost allways is slower than iteration. You can read more about it in this post or in this quora thread (yes, I know, its quora, but this answer there is good).

Instead of using signals you could override the models save method.

models.py

from .utils import unique_order_reference_generator

class ReservedItem(models.Model):
    ...

    def save(self, *args, **kwargs):
        if not self.order_reference:
            self.order_reference = unique_order_reference_generator()
        super().save(*args, **kwargs)

Of course, there is the question of why you are doing the random generation at all. Can you not use a sequential id, like djangos pk-by-default AutoField?

You could even just get the biggest number so far and increment it (asuming that order_reference is a numeric field):

max_1 = ReservedItem.objects.all().aggregate(
    Max('order_reference'))['order_reference__max']
max_2 = Order.objects.all().aggregate(
    Max('order_reference'))['order_reference__max']
new_order_reference = max(max_1, max_2) + 1

Thank you so much Ralf. I will solve it now with the ´save´ method. The reason I didn't use the primary key is that this order id (together with a customer token) will be used to display event tickets. pk's/numbers you could always guess correctly. Adding some letters, it makes it way more difficult. That was my reasoning behind it. — , Jun 19 '18 at 06:40

Django signals > right approach?

1 Answers1