I think my code is pretty ugly...advice appreciated :-)

Using Django Getting Started
1

Hi there once more,

today I don’t need help to achieve a goal, my code already works…but I can’t help thinking about, that it is not the pythonic way and not the “DoNotRepeatYourself” Way of Django to do it like this…but it was the first solution that came to my mind. However, I am not very happy about. What I want to do, is according to some law regulations display a page with a summary of Users/Roles/Rights.

I managed to do that with the following code in views.py:

def rights(request):
    context = {
        'groups': Group.objects.all(),
        'admin': User.objects.filter(groups__name='admin').order_by('last_name'),
        'examiner': User.objects.filter(groups__name='examiner').order_by('last_name'),
        'manv': User.objects.filter(groups__name='manv').order_by('last_name'),
        'office': User.objects.filter(groups__name='office').order_by('last_name'),
        'view_all': User.objects.filter(groups__name='view_all').order_by('last_name'),
        'view_examinations': User.objects.filter(groups__name='view_examinations').order_by('last_name'),
        'view_retired': User.objects.filter(groups__name='view_retired').order_by('last_name')
    }

    return render(request, 'rights.html', context)

and this code in template:

{%  for group in groups %}
        <div class="col">
            <div class="card h-auto border-dark border-2 bg-light">
                <div class="card-header text-center fs-4 text-white bg-primary bg-gradient"><b>{{ group.name }}</b></div>
                <div class="card-text m-1 ps-1">
                {% if group.name == 'admin' %}
                    {% for user in admin %}
                        <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                    {% endfor %}
                {% endif %}
                {% if group.name == 'examiner' %}
                    {% for user in examiner %}
                        <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                    {% endfor %}
                {% endif %}
                {% if group.name == 'manv' %}
                    {% for user in manv %}
                        <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                    {% endfor %}
                {% endif %}
                {% if group.name == 'office' %}
                    {% for user in office %}
                        <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                    {% endfor %}
                {% endif %}
                {% if group.name == 'view_all' %}
                    {% for user in view_all %}
                        <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                    {% endfor %}
                {% endif %}
                {% if group.name == 'view_examinations' %}
                    {% for user in view_examinations %}
                        <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                    {% endfor %}
                {% endif %}
                {% if group.name == 'view_retired' %}
                    {% for user in view_retired %}
                        <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                    {% endfor %}
                {% endif %}
                </div>
            </div>
        </div>
        {% endif %}
    {% endfor %}

so in the end, i get what i wanted, but it seems not very elegant to me :frowning:

2

I’ve seen worse (much worse) :slightly_smiling_face:. This actually isn’t all that bad.

First, and just to let you know, I’m winging this - there may be an error or 10 lurking in the code. If you try to adapt this, be prepared to debug my mistakes - and don’t blame me if it makes your computer melt.

So my first step is to produce a list with the groups in the order you have them defined:
group_list = ['admin', 'examiner', 'manv', 'office', 'view_all', 'view_examinations', 'view_retired'] (I think I got them all)

Now I’m going to create a dict of all the users for each group:

member_list = {
    group_name: User.objects.filter(groups__name=group_name).order_by('last_name')
    for group_name in group_list
}

The context I pass to render is then:
context = { 'member_list': member_list }

My template then looks something like this:

{%  for group_name, members in member_list.items %}
    <div class="col">
        <div class="card h-auto border-dark border-2 bg-light">
            <div class="card-header text-center fs-4 text-white bg-primary bg-gradient"><b>{{ group_name }}</b></div>
            <div class="card-text m-1 ps-1">
                {% for user in members %}
                    <nobr>{{ user.username }} - {{ user.last_name }}, {{ user.first_name|slice:1 }}.</nobr><br>
                {% endfor %}
            </div>
        </div>
    </div>
{% endfor %}

Side note: There’s actually a way to improve the queries a bit, we can get into that later if necessary.

6 Likes
3

Hi Ken,

I´m sorry to dissapoint You but no matter how hard I tried to debug your mistakes…there weren’t any :wink:
Neither did my machine melt down…seems it’s a lucky day :slight_smile:

There is no urgent need to improve queries actually, however I’m always happy to learn things! :wink:

1 Like
4

So the issue with this construct:

is that you’re actually issuing separate queries to the database for each group. This means that in this particular case, you’re sending 7 separate queries to the database. If you had 20 groups, that would be 20 queries.

One way to improve this will be to do this all in one query, so we’re going to change this a bit. We’ll take advantage of the standard JSONObject function along with the PostgreSQL-specific JSONBAgg function to annotate the list of users in a group, with the group, as part of the query

from django.db.models.functions import JSONObject
from django.contrib.postgres.aggregates import JSONBAgg

...

member_list = {
  group['name']: group['users'] 
  for group in Group.objects.filter(name__in=group_list
  ).annotate(
    users=JSONBAgg(
      JSONObject(
        username=F('user__username'),
        last_name=F('user__last_name'),
        first_name=F('user__first_name')
      )
    )  
  ).values('name', 'users')
}

We’re letting the database organize the data, and then creating our members_list from the pre-organized data.

3 Likes
5

I thank you very much for this hint. I am always a fan of learning things and altering the code towards higher perfomance is also always very appreciated. As you wrote about Postgres-Specific function, I assume, this approach will not work with a MySQL Database ?

6

I honestly don’t know. I don’t know what facilities may be available in MySQL to do something comparable to this. I don’t think these types of functions exist in the standard MySQL driver module, but that doesn’t mean that those functions aren’t available in the database to be implemented.

7

Ok, thanks for this hint anyway…at the moment, performance issues are not in my focus, but I will check this after the basic-work is done :slight_smile: