Unexpected Python result to Code

Question

dictionary = {'Jason Seifer': ['Ruby Foundations', 'Ruby on Rails Forms', 'Technology Foundations'], 'Kenneth Love': ['Python Basics', 'Python Collections'], 'Daniel': ['Python Basics', 'Python Collections', 'test', 'test']}

def most_class(dictionary):
    current_count = 0
    max_count = 0
    best_teacher = "none"
    for key in dictionary:
        for values in dictionary.values():
            current_count += 1
            if current_count > max_count:
                max_count = current_count
                best_teacher = key
    return best_teacher

The goal is to determine which key in the dictionary has the most values. The answer should be Daniel, but I am getting a different answer each time I run the code (presumably due to pythons hashing of the dictionary.

Can anyone explain why I am getting this result, and how it can be fixed?

icktoofay · Accepted Answer · 2014-12-24 02:08:01Z

1

You have two problems.

You need to reset current_count each time around, not just at the start.
dictionary.values() gives you the list of lists of books, not the list of books for that author.

To solve the second one, I would use for author, books in dictionary.items(): rather than for key in dictionary:. Then you won’t have to use dictionary[key] each time. After making these changes, we have:

def most_class(dictionary):
    max_count = 0
    best_teacher = "none"
    for author, books in dictionary.items():
        current_count = 0
        for book in books:
            current_count += 1
            if current_count > max_count:
                max_count = current_count
                best_teacher = author
    return best_teacher

Further, I might pull the if out of the loop:

def most_class(dictionary):
    max_count = 0
    best_teacher = "none"
    for author, books in dictionary.items():
        current_count = 0
        for book in books:
            current_count += 1
        if current_count > max_count:
            max_count = current_count
            best_teacher = author
    return best_teacher

At that point, it becomes clear that we don’t need a for loop at all, and can just use len:

def most_class(dictionary):
    max_count = 0
    best_teacher = "none"
    for author, books in dictionary.items():
        if len(books) > max_count:
            max_count = len(books)
            best_teacher = author
    return best_teacher

Your code might also benefit from using the max function as others have suggested, although I would probably use it a different way:

def most_class(dictionary):
    return max(dictionary.items(), default=('none', 0),
               key=lambda item: len(item[1]))[0]

edited Dec 24, 2014 at 2:08

answered Dec 24, 2014 at 1:58

icktoofay

130k23 gold badges261 silver badges239 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

user3839821 Over a year ago

Thakns for this answer, I believe it finally works. I am still learning python, so I am not sure what you did in line 4. Could you explain what is going on? I would have thought that would result in a syntax error, is python assuming you are picking the values in a key for each of the dictionary items? Thanks.

icktoofay Over a year ago

@user3839821: I’m not sure if you’ve learned tuples yet, but they look like this: (1, 2, 3). Iterating over the return value of items, you get tuples of (key, value). It turns out Python has a handy ‘unpacking’ syntax, where if you have for a, b in it:, that’s roughly equivalent to for tup in it: a = tup[0]; b = tup[1]. So by doing that, we’re iterating over all of the keys and values in the dictionary simultaneously, putting each key in author and its associated value in books.

skrrgwasme · Accepted Answer · 2014-12-24 02:13:26Z

1

You're iterating over the dictionary more times than you need to, and making this much more difficult than it needs to be. You can do it much more simply by combining the Python built-in max() with comprehensions. Try this instead:

dictionary = {
    'Jason Seifer': ['Ruby Foundations', 'Ruby on Rails Forms',
                     'Technology Foundations'], 
    'Kenneth Love': ['Python Basics', 'Python Collections'], 
    'Daniel': ['Python Basics', 'Python Collections', 'test', 'test']}

# get maximum number of items any one person has
maximum_len = len(max(dictionary.values(), key=len))

# build a list of everyone with that number of items
most = [name for name, classes in dictionary.items() 
            if len(classes) == maximum_len]

print(most)

This code prints:

['Daniel']

Not only does this replace loops with comprehensions, in the event of a tie, you get a list of the names instead of just the first one found. For example, if we change the dictionary definition to this:

dictionary = {
    'Jason Seifer': ['Ruby Foundations', 'Ruby on Rails Forms', 
                     'Technology Foundations', 'other_item'], 
    'Kenneth Love': ['Python Basics', 'Python Collections'], 
    'Daniel': ['Python Basics', 'Python Collections', 'test', 'test']}

Then this will be printed instead:

['Daniel', 'Jason Seifer']

edited Dec 24, 2014 at 2:13

answered Dec 24, 2014 at 1:54

skrrgwasme

9,67112 gold badges58 silver badges87 bronze badges

2 Comments

user3839821 Over a year ago

Thanks. I am unsure how I could use this within my code, do you mind being a bit more verbose? Similarly, I am still unsure why my code does NOT work, even with the process being more difficult.

mattsilver Over a year ago

Your code doesn't work because you should have for values in dictionary[key] I think. Either way, this answer is on the right track. You definitely don't need for loops for this

Burger King · Accepted Answer · 2014-12-26 00:12:27Z

1

You have to know how to use dictionary , then I recommend to do:

dictionary = {'Jason Seifer': ['Ruby Foundations', 'Ruby on Rails Forms', 'Technology Foundations'], 'Kenneth Love': ['Python Basics', 'Python Collections'], 'Daniel': ['Python Basics', 'Python Collections', 'test', 'test']}

def most_class(dictionary):
    best_teacher = None
    class_of_best_teacher = 0
    for teacher_name in dictionary.keys():
        class_of_teacher = len(dictionary[teacher_name])
        if class_of_teacher > class_of_best_teacher :
            best_teacher = teacher_name
            class_of_best_teacher = class_of_teacher
    return best_teacher
print most_class(dictionary)

edited Dec 26, 2014 at 0:12

answered Dec 24, 2014 at 2:22

Burger King

2,9753 gold badges23 silver badges47 bronze badges

Comments

dragon2fly · Accepted Answer · 2014-12-24 02:09:49Z

0

you iterates through keys in the dictionary but you for got to reset your current_count back to zero between each key. So current_count and max_count counts up to 9 together and best_teacher will be the last key that the for loop iterates through.

Fix your code like this.

...
for key in dictionary:
    current_count = 0
    ...

It work but I would like to do this instead:

best_teacher = sorted(dictionary.keys(), key=lambda x: len(dictionary[x]))[-1]

answered Dec 24, 2014 at 2:09

dragon2fly

2,45921 silver badges24 bronze badges

Comments

Lax · Accepted Answer · 2014-12-24 02:11:57Z

reset current_count for each key
should use dictionary[key] instead of dictionary.values()

the following code return Daniel.

dictionary = {'Jason Seifer': ['Ruby Foundations', 'Ruby on Rails Forms', 'Technology Foundations'], 'Kenneth Love': ['Python Basics', 'Python Collections'], 'Daniel': ['Python Basics', 'Python Collections', 'test', 'test']}

def most_class(dictionary):
    current_count = 0
    max_count = 0
    best_teacher = "none"
    for key in dictionary:
        current_count = 0
        for values in dictionary[key]:
            current_count += 1
            if current_count > max_count:
                max_count = current_count
                best_teacher = key
    return best_teacher

Collectives™ on Stack Overflow

Unexpected Python result to Code

5 Answers 5

2 Comments

2 Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

2 Comments

2 Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related