11

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)
MSeifert
  • 145,886
  • 38
  • 333
  • 352
zenprogrammer
  • 623
  • 1
  • 7
  • 20
  • 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. – timgeb Jun 09 '17 at 13:12
  • 1
    This looks good as is. Using 3 `for` loops in list comprehension just becomes an ugly mess. – Remolten Jun 09 '17 at 13:14

4 Answers4

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)
timgeb
  • 76,762
  • 20
  • 123
  • 145
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)]
MSeifert
  • 145,886
  • 38
  • 333
  • 352
  • 2
    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. – AChampion Jun 09 '17 at 13:17
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.

MSeifert
  • 145,886
  • 38
  • 333
  • 352
-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]
Bubble Bubble Bubble Gut
  • 3,280
  • 12
  • 28