Mdthbs

Given a text like "Hello World!" the program outputs the most frequent letter, in this case the "l".

If there are two letters with the same frequency, it outputs the first in alphabetical order, for example in the text "One" it outputs "e".

In addition upper and lower letters are counted as the same letter.

How could be this code improved?

def most_frequent_letter(text: str) -> str:



    letters_dict = {}

    text = text.lower()

    text = re.sub("[^a-z]","",text)



    for letter in text:

        if letter in letters_dict:

            letters_dict[letter] += 1

        else:

            letters_dict[letter] = 1



    value_to_key = collections.defaultdict(list)

    for key_letter, frecuency_value in letters_dict.items():

        value_to_key[frecuency_value].append(key_letter)



    letters = list(value_to_key.values())

    frecuencies = list(value_to_key.keys())

    return (sorted(letters[frecuencies.index(max(frecuencies))])[0])

If you are going to use the collections module at all (as you did for collections.defaultdict), then why not use collections.Counter, which offers a most_common() method?

For the most part your code is okay, however, reading what it is that you want it to do here's what I came up with...

#!/usr/bin/env python3



import re

import typing





def most_frequent_letter(text: str) -> typing.Tuple[int, list]:

    text = re.sub('[^a-z]', '', text.replace(' ', '').lower())



    letter_counts = {}

    for letter in text:

        letter_counts[letter] = letter_counts.get(letter, 0) + 1



    counted_letters = {}

    for letter, counter in letter_counts.items():

        counted_letters[counter] = counted_letters.get(counter, []) + [letter]



    max_key = max(counted_letters.keys())

    return max_key, sorted(counted_letters[max_key])





if __name__ == '__main__':

    counted, letters = most_frequent_letter('spam ham jello spam')

    print("{counted} -> {letter}".format(

        counted = counted,

        letter = letters[0]))

... while I don't know that it's an improvement overall it does return results that may be of use...

3 -> a

Note if ya want the whole list use the following instead under that last if statement...

# ...

    print("{counted} -> {letters}".format(

        counted = counted,

        letters = letters))

# ... Should output...

#     3 -> ['a', 'm']

... while also not counting spaces as letters; which is the only thing that seemed missing from your implementation.

I didn't see any need to use if statements (within the function) so didn't include'em, because dictionaries have a handy get method that allows for defaulting nonexistent key value pares. And I didn't see any use in using collections though it was tempting to use @200_success's suggestion.

One thing about your code that I do still question though is why are ya wrapping the returned value within parentheses?

There are many ways to do this shorter. For example, you can use the Counter class (in Python 2.7 or later):

import collections

s = "helloworld"

print(collections.Counter(s).most_common(1)[0])

If you don't have that, you can do the tally manually (2.5 or later has defaultdict):

d = collections.defaultdict(int)

for c in s:

    d[c] += 1

print(sorted(d.items(), key=lambda x: x[1], reverse=True)[0])

Having said that, there's nothing wrong with your implementation.

NOTE - This prints the letter along with how many times it actually occurs.

Hope this helps!