4
\$\begingroup\$

I'm creating a django web application and I would like to know how to shorten this code.

I have a model where every user has a typeOfPerson (Student, Teacher, Guest ...) and I want to count these types.

@register.simple_tag()
def get_stats_type(choice):
    items = []
    for vote in choice.vote_set.all():
        if vote.user not in items:
            items.append(vote.user)

    types = {}
    for user in items:
        if user.userprofile.typeOfPerson not in types:
            types[user.userprofile.typeOfPerson] = Vote.objects.filter(Choice=choice).count()

    percent = 0
    for type, value in types.items():
        percent += value
    if percent == 0:
        percent = 1

    values = {}
    for type, value in types.items():
        if type not in values:
            values[type] = value / percent * 100

    return values
\$\endgroup\$
3
\$\begingroup\$

I'm creating a django

You probably mean that you're creating a web application on top of Django.

Given this code:

items = []
for vote in choice.vote_set.all():
    if vote.user not in items:
        items.append(vote.user)

you should instead be able to do

items = set(v.user for v in choice.vote_set.all())

This achieves the same goal - creating a collection of unique entries. The only catch is that order will not be preserved. If that's important, you'll have to do something more clever.

This:

types = {}
for user in items:
    if user.userprofile.typeOfPerson not in types:
        types[user.userprofile.typeOfPerson] = Vote.objects.filter(Choice=choice).count()

apparently makes an effort to avoid overwriting entries in a dictionary, but I'm not clear on whether that's necessary. If order doesn't matter, and if items all have equal priorities, then you can make this much more simple:

n_votes = Vote.objects.filter(Choice=choice).count()
types = {u.userprofile.typeOfPerson: n_votes for u in items}

This also assumes that Vote.objects.filter(Choice=choice).count() doesn't vary at all over the course of the loop.

This:

percent = 0
for type, value in types.items():
    percent += value
if percent == 0:
    percent = 1

can be:

percent = max(1, sum(types.values()))

This:

values = {}
for type, value in types.items():
    if type not in values:
        values[type] = value / percent * 100

can probably just be

values = {t: v/percent * 100 for t, v in types.items()}

Again, it's not clear why all three of your loops are attempting to be careful in avoiding rewriting values, or what the priority system should be.

|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

This code itself doesn't look bad, but since this is Code Review there are always ways that it can be improved.

Let's talk about what the code was supposed to do

PEP 8, the Python style guide, strongly encourages docstrings to be used to document what the function itself is supposed to do. Since this is a Django template tag, docstrings can be used here as well. I wrote up a small one based on my understanding of what is trying to be accomplished here.

@register.simple_tag()
def get_stats_type(choice):
    """
    Returns a dictionary containing the percentage of users who voted for a
    choice, organized by the type of user, out of all users who voted.
    """

It doesn't have to be anything long or complex, but the idea is to document what is trying to be accomplished so future readers of your code can understand it more quickly.

Spoiler: your code doesn't currently do what this docstring says.

Document what you're actually trying to accomplish along the way

Again, documentation is something which someone will inevitably jump on here and should "But you don't need it if your code is obvious!" and they might be right. But hey, it's Code Review and here's an example of why you might want it.

The first part of your function is trying to get all of the users who voted for the choice that was passed in. In the Django ORM, depending on what you're looking to get back, you can shortcut this in a few different ways.

If you're just trying to get the IDs of all of the users, you query for all of the votes with that choice and use .values_list('user', flat=True) to get the user ID back.

items = Vote.objects.filter(Choice=choice).values_list('user', flat=True).distinct()

And this will return a list of user IDs for all the unique votes who voted for this choice. But, based on the next part of the function, it actually looks like you may be looking for the types of the users who voted for this choice, which is a little more complex but can also be done on the database side.

# Get all of the users who voted for this choice
users = Vote.objects.filter(Choice=choice).values_list('user', flat=True).distinct()

# Get a list of the types of the users who voted
user_types = UserProfile.objects.filter(users__in=users).values_list('typeOfPerson', flat=True).distinct()

This will get you a list of the unique user types who voted using this choice.

You aren't getting the votes for just the specific user type

This was caught in the other answer, but it was never addressed and was treated as something which could be simplified. Right now, any time this function returned percentages, all of the user types will have an equal percentage because it uses the same set of votes for each user type, never filtering it down.

votes_by_type = {}

# Get the number of votes, by user type, who voted for this choice
for user_type in user_types:
    votes_by_type[user_type] = Vote.objects.filter(
        user__userprofile__typeOfPerson=user_type,
        Choice=choice,
    ).count()

While this can be done with a dictionary comprehension, as recommended by the other answer, I would suggest doing it as you did it before. It's easier to read and understand what is actually trying to be accomplished.

Here is where the bug that existed in the original version was fixed. I added an additional filter() to the votes to limit it down to just those made by the given user type.

We don't need to check for duplicate user types here because we know that user_types is a list of unique user types already.

Summing it up and doing percentages

Python has a built-in sum function which can be used to get the sum of a list of numbers. Dictionaries in Python also have a built-in values function which gets you the list containing all of the values in the dictionary. You can combine these together to get the total votes cast for the choice.

# Get the total number of votes cast for this choice
total_votes = sum(votes_by_type.values())

You can then use this, as stated in the other answer and done originally, to calculate the percentages.

percentages = {}

# Calculate the percentage of votes by user type
for user_type in user_types:
    percentages[user_type] = (votes_by_type[user_type] / total_votes) * 100

But wait, I'm not checking for when total_votes is zero! This is because if nobody votes, then user_types will be empty (because nobody voted, so there are no user types who voted), and the division by zero error will not happen.

If you choose to change this later to always return the types regardless of whether or not they have actually voted, then you may need to add back in your logic of handling cases where there are no votes. But I would consider this to be a standard special case, since you're trying to calculate percentages based off of no votes.

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.