# Counting users by the type in their profile

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


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.

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:
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


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:

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.