12

I have to create a list of blocked users per key. Each user has multiple attributes and if any of these attributes are in keys, the user is blocked.

I wrote the following nested for-loop and it works for me, but I want to write it in a more pythonic way with fewer lines and more readable fashion. How can I do that?

for key in keys:
    key.blocked_users = []

for user in get_users():
    for attribute in user.attributes:
        for key in keys:
            if attribute.name == key.name:
                key.blocked_users.append(user)
2
  • 4
    I don't think your code is unpythonic. I also don't think that saving lines necessarily makes code more pythonic. Anyway: Usually you can use itertools.product to "avoid" nested for loops, but not if the inner loop relies on the outer loop variable. Commented Jun 9, 2017 at 13:12
  • 1
    This looks good as is. Using 3 for loops in list comprehension just becomes an ugly mess. Commented Jun 9, 2017 at 13:14

4 Answers 4

7

In your specific case, where the inner for loops rely on the outer loop variables, I'd leave the code just as is. You don't make code more pythonic or readable by forcefully reducing the number of lines.

If those nested loops were intuitively written, they are probably easy to read.

If you have nested for loops with "independent" loop variables, you can use itertools.product however. Here's a demo:

>>> from itertools import product
>>> a = [1, 2]
>>> b = [3, 4]
>>> c = [5]
>>> for x in product(a, b, c): x
... 
(1, 3, 5)
(1, 4, 5)
(2, 3, 5)
(2, 4, 5)
Sign up to request clarification or add additional context in comments.

3 Comments

Agreed, using itertools.product(get_users(), keys) still leaves an awkward key.blocked_users creation.
@AChampion As I said, I don't recommend the use of itertools.product in OP's case.
Sorry, meant that as an agreement.
5

You could use a conditional comprehension in your first for-loop:

for key in keys:
    keyname = key.name
    key.blocked_users = [user for user in get_users() if any(attribute.name == keyname for attribute in user)]

1 Comment

Exactly as I wrote it, though would pull out users = get_users() out of the loop if it is an expensive operation. I didn't create keyname but it's a minor performance improvement.
4

Aside from making it shorter, you could try to reduce the operations to functions that are optimized in Python. It may not be shorter but it could be faster then - and what's more pythonic than speed?. :)

For example you iterate over the keys for each attribute of each user. That just sreams to be optimized "away". For example you could collect the key-names in a dictionary (for the lookup) and a set (for the intersection with attribute names) once:

for key in keys:
    key.blocked_users = []

keyname_map = {key.name: key.blocked_users for key in keys}  # map the key name to blocked_user list
keynames = set(keyname_map)

The set(keyname_map) is a very efficient operation so it doesn't matter much that you keep two collections around.

And then use set.intersection to get the keynames that match an attribute name:

for user in get_users():
    for key in keynames.intersection({attribute.name for attribute in user.attributes}):
        keyname_map[key].append(user)

set.intersection is pretty fast too.

However, this approach requires that your attribute.names and key.names are hashable.

Comments

-1

Try using listed for loop in list comprehension, if that's considered more Pythonic, something like:

[key.blocked_users.append(user) for key in keys 
        for attribute in user.attributes 
        for user in get_users() 
        if attribute.name == key.name]

3 Comments

I don't think this is more readable. It's not even much shorter when you wrap your lines at 80 characters.
I am not sure that a list comprehension long like this is really more readable.
And using a list comprehension for side effects seems like an abuse.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.