9
\$\begingroup\$

I'm getting the highest value and its respective from a dictionary and printing it out for the user.

Even though it's working, I'm wondering if I'm doing it in the cleanest way. Here it is:

People = {'Harry': 20, 'Ron': 19, 'Hermione': 21, 'Hagrid': 54}

print(max(People, key=People.get), max(People.values()))

# Result: Hagrid 54

What would you do differently from this?

\$\endgroup\$
0
20
+50
\$\begingroup\$

What would you do differently from this?

  • PEP 8: Variable names should be lowercase. People -> people.

  • As @FMc suggested in the comment, better to separate computation and printing. It is easier to understand and change.

  • An alternative (that I find more readable) is to use a lambda in the max function and unpack the tuple afterward:

    people = {'Harry': 20, 'Ron': 19, 'Hermione': 21, 'Hagrid': 54}
    entry = max(people.items(), key=lambda x: x[1])
    print(*entry)
    # Result: Hagrid 54
    

    Or unpacking directly:

    key, value = max(people.items(), key=lambda x: x[1])
    print(key, value)
    # Result: Hagrid 54
    

Edit:

Using operator.itemgetter, as suggested in the comments:

import operator

people = {'Harry': 20, 'Ron': 19, 'Hermione': 21, 'Hagrid': 54}
key, value = max(people.items(), key=operator.itemgetter(1))
print(key, value)
# Result: Hagrid 54
\$\endgroup\$
2
  • 2
    \$\begingroup\$ More readable - and likely more efficient, since we only invoke max() once here. \$\endgroup\$ Jun 18 at 7:14
  • 2
    \$\begingroup\$ I knew sort had a key parameter, but didn't know max had it too. Learn something new every day. \$\endgroup\$ Jun 18 at 16:02
8
\$\begingroup\$

I don't know what your value represents, so I will assume age.

Since you already have an implementation ready, which is good enough. I think just wrapping the code in a function would increase the readability of your code. This way you're providing a descriptive API for the functionality. You can then reuse this in multiple places and change the implementation however you want.

Then the implementation becomes an "implementation detail" and has nothing to do with the cleanest way. So you will not have to figure out what max in print(max) does, each time you read the code. Regardless of how good you can make max look, a function will always be more readable. As when you read the code, you read it like:

entry is max from people with key of entry [1]. Oh ok, you get the entry is the entry with the max value, whatever that means

print(oldestPerson(People))

How you implement the oldestPerson() doesn't really matter and you will be able to refactor it however you want later.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ @Peilonrayz, your edit greatly enriched the answer. Thank you very much for this and the clarifications. \$\endgroup\$
    – Arnon
    Jun 18 at 23:39
5
\$\begingroup\$

There are many, many different ways to do this. If I had to pick a favourite for legibility, it would be to apply a single max to a generator that reverses the key and value (max_gen, below). If you need to squeeze out every last microsecond of performance, writing your own loop is faster. Whatever you do, don't call max twice.

Side-by-side

from functools import partial
from operator import itemgetter
from random import shuffle
from timeit import timeit
from typing import Dict, Tuple
from uuid import uuid4

AgeDict = Dict[
    str,  # name
    int,  # age, years
]
AgeTuple = Tuple[str, int]


def max_getter(people: AgeDict) -> AgeTuple:
    return max(people.items(), key=itemgetter(1))


def max_lambda(people: AgeDict) -> AgeTuple:
    return max(people.items(), key=lambda kv: kv[1])


def max_value(people: AgeDict) -> AgeTuple:
    max_v = max(people.values())
    max_k = next(k for k,v in people.items() if v==max_v)
    return max_k, max_v


def max_gen(people: AgeDict) -> AgeTuple:
    # My favourite for legibility
    age, name = max((age, name) for name, age in people.items())
    return name, age


def max_loop(people: AgeDict) -> AgeTuple:
    items_iter = iter(people.items())
    max_name, max_age = next(items_iter)
    for name, age in items_iter:
        if max_age < age:
            max_age = age
            max_name = name
    return max_name, max_age


def max_invert(people: AgeDict) -> AgeTuple:
    inverted = {v: k for k, v in people.items()}
    max_age = max(inverted.keys())
    return inverted[max_age], max_age


METHODS = (
    max_loop,
    max_value,
    max_getter,
    max_gen,
    max_lambda,
    max_invert,
)


def test():
    d = {'a': 3, 'b': 1, 'c': 4, 'd': 2}
    for method in METHODS:
        assert method(d) == ('c', 4)


def time():
    n = 10_000
    ages = list(range(n))
    shuffle(ages)
    people = {uuid4(): age for age in ages}
    reps = 200

    for method in METHODS:
        print(f'{method.__name__:16}', end=' ')
        f = partial(method, people)
        t = timeit(f, number=reps) / reps
        print(f'{t*1e6:>4.0f} us')


if __name__ == '__main__':
    test()
    time()

Output

max_loop          388 us
max_value         458 us
max_getter        571 us
max_gen           976 us
max_lambda        976 us
max_invert        961 us
\$\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.