3
\$\begingroup\$

I'd like feedback on my solution to the outlined programming challenge (medium level). I've tried it fast in any way I know how to, but, what might be a more efficient and/or pythonic solution?

[Letter Count II] Have the function LetterCount(str) take the str parameter being passed and return the first word with the greatest number of repeated letters. For example: "Today, is the greatest day ever!" should return greatest because it has 2 e's (and 2 t's) and it comes before ever which also has 2 e's. If there are no words with repeating letters return -1. Words will be separated by spaces.

import doctest
import logging
import timeit

from collections import Counter
from string import ascii_letters

def letter_count(s: str):
    """Return the word in s with the largest number of repeating letters


    >>> letter_count(s="Today, is the greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the >>>>greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever xx!")
    'greatest'
    >>> letter_count(s="")
    -1
    >>> letter_count(s="abcd efg hijk")
    -1
    """
    if s == "":
        return -1

    word_counters = dict((word, Counter(word)) for word in s.split(" "))

    # tuple of (word(str), most common letter frequency(int))
    most_repeating_word = ("", 0)

    for word, counter in word_counters.items():

        # most_common format = (letter(str), freq(int))
        for most_common in counter.most_common():

            if most_common[0] not in ascii_letters:
                continue

            if most_common[1] > most_repeating_word[1]:
                most_repeating_word = (word, most_common[1])
                break

    if most_repeating_word[1] <= 1:
        return -1

    return "".join(l for l in most_repeating_word[0] if l in ascii_letters)


if __name__ == "__main__":
    doctest.testmod()
    print(timeit.timeit("letter_count(s='!!!Today, is the greatest >>>> day ever!')",
                  setup=("from __main__ import letter_count; from collections "
                         "import Counter; from string import ascii_letters"),
                  number=10000))
\$\endgroup\$
5
\$\begingroup\$

The core problem can be solved a lot quicker using the built-in max and making a generator that produces the maximum count of repeated letters. Note that max already takes the first occurrence of the maximum, as required by the challenge, but we need to take care to not compare the words lexicographically, as I did in a previous version, by using a key function. You should probably add testcases for this, such as "free greatest" -> "free" and "greatest free" -> "greatest".

from collections import Counter
from operator import itemgetter

def letter_count(s):
    words = s.split(" ")
    most_common_letters = (Counter(word).most_common(1)[0][1] for word in words)
    return max(zip(most_common_letters, words), key=itemgetter(0))[1]

This does not handle any of the edge cases, though. It fails if the most common letter is not an ASCII character, but we can easily get around this by removing all of those right at the start. I also added your check against the empty string, but improving it to exit early if there is nothing left after filtering for ASCII characters, including the case that only spaces are left. I also split on all whitespace, not just single spaces, because this way multiple spaces are joined and all other whitespace characters have already been removed. I also use if not s instead of if s == "", because empty sequences are falsey and it sounds more like English this way.

from collections import Counter
from string import ascii_letters
from operator import itemgetter

ASCII_LETTERS = set(ascii_letters) | {" "}

def letter_count(s: str):
    """Return the word in s with the largest number of repeating letters


    >>> letter_count(s="Today, is the greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the >>>>greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever xx!")
    'greatest'
    >>> letter_count(s="Today, is thee greatest day ever!")
    'thee'
    >>> letter_count(s="!!! >>>>")
    -1
    >>> letter_count(s="")
    -1
    >>> letter_count(s="abcd efg hijk")
    -1
    """
    s = "".join(c for c in s if c in ASCII_LETTERS)
    if not s.replace(" ", ""):
        return -1
    words = s.split()
    most_common_letters = (Counter(word).most_common(1)[0][1] for word in words)
    best_word = max(zip(most_common_letters, words), key=itemgetter(0))
    return best_word[1] if best_word[0] > 1 else -1

Note that the challenge design is not the best. Normally I would recommend against returning differing types from a function unless absolutely necessary. Having it return a string in most cases and sometimes an int can be dangerous. Better to at least return None, if you absolutely need a special value, or even better, just let the caller deal with an exception.

Otherwise your code looks quite alright. You have a docstring, tests and a main guard. I must say that your comments don't help me a lot, I personally prefer plain English or, even better, self-explanatory variable names. But naming things is hard, and you have already done a good enough job at that.

| improve this answer | |
\$\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.