8
\$\begingroup\$

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])
\$\endgroup\$
12
\$\begingroup\$

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?

\$\endgroup\$
5
\$\begingroup\$

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

import collections
s = "helloworld"
print(sorted(collections.Counter(s).most_common(1)))

You can use the sorted() function if you want to print the output in alphabetical order.

Here are some example outputs:

#s = "helloworld"

#print(sorted(collections.Counter(s).most_common(1)))

>>> [('l', 3)]

#s = "hellowaaald"

#print(sorted(collections.Counter(s).most_common(2)))

>>> [('a', 3), ('l', 3)]

You can change the value of n in .most_common(n) depending upon how many "most common elements" you need.

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!

\$\endgroup\$
3
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

Good job so far, your code appears to be correct. I have a few comments, mostly on style.

First, using the typing, re, and collections modules is perhaps somewhat overkill for what you are trying to achieve. For small programs, it's best to make use of the built-in functions, and methods of build-in types, rather than to import modules, if possible. In particular, the string methods can be useful, and I've found the max, map, all, and any functions to be handy.

Second, while it wasn't specified what the function should do given an empty string, it would be prudent to handle this case. Currently, your code will throw an error; I think it would make sense to return the letter 'a'.

Third, parts of your code are a bit difficult to read. In particular, this expression

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

required some concentration to understand. I'm not sure how this could be improved. I think the problem is that by this point, with the re-jigging and type conversions, I've lost track of what the data-structures look like.


That's it for comments. To finish, here is an approach to the problem that I think would be considered quite Pythonic:

def most_frequent_letter(text):
    """Return the first letter of the alphabet that occurs jointly most 
    often in the given string. Case is ignored."""
    alphabet = 'abcdefghijklmnopqrstuvwxyz'
    return max(alphabet, key=text.lower().count)

The max function will return the first maximal element in the list, where the value being maximised is the result of applying the function key. So in this case, it will return the alphabetically first letter with the (joint) highest count in the lowercased text.

You could import the latin alphabet from the string module, if you'd rather not type it out:

from string import ascii_lowercase

Good job on producing a correct solution to a tricky problem. I would work on:

  • becoming more familiar with the built-in methods and functions;
  • taking a step back to consider whether there is a more elegant way to approach the problem rather than ploughing on; and
  • writing code with the human reader in mind: try to keep the state of variables simple and self-evident. Ideally a line of code should be somewhat comprehensible in isolation (without having to use working memory to mentally build up the state of the variables involved from the previous lines of the program).
\$\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.