9
\$\begingroup\$

I would appreciate some feedback on the below code I wrote for a Project Euler problem. The answer comes out correct but I'd like feedback on the design of the function and implementation of docstrings. It seems to me that it may be overly complicated but I'm not sure how to simplify.

"""
Number letter counts

Problem 17
If the numbers 1 to 5 are written out in words: one, two, three, four, five, then there are
3 + 3 + 5 + 4 + 4 = 19 letters used in total.

If all the numbers from 1 to 1000 (one thousand) inclusive were written out in words, how many
letters would be used?


NOTE: Do not count spaces or hyphens. For example, 342 (three hundred and forty-two) contains
23 letters and 115 (one hundred and fifteen) contains 20 letters. The use of "and" when writing
out numbers is in compliance with British usage.
Answer = 21124
"""

num_word_map = {0: '', 1: 'one', 2: 'two', 3: 'three', 4: 'four', 5: 'five',
                6: 'six', 7: 'seven', 8: 'eight', 9: 'nine', 10: 'ten',
                11: 'eleven', 12: 'twelve', 13: 'thirteen', 14: 'fourteen',
                15: 'fifteen', 16: 'sixteen', 17: 'seventeen', 18: 'eighteen',
                19: 'nineteen', 20: 'twenty', 30: 'thirty', 40: 'forty',
                50: 'fifty', 60: 'sixty', 70: 'seventy', 80: 'eighty',
                90: 'ninety', 100: 'hundred', 1000: 'thousand'}


def convert_to_word(x, spacing=''):
    """
    Recursive function that converts number to it's equivalent word up to a limit of 9999.
    Function is called recursively until x < 20.

    Parameters
    ----------
    x : number to convert to word
    spacing : keeps track appropriate value to enter between words. This will be 'and' or an empty string

    Returns
    -------
    Equivalent word for the number x

    """
    if x / 1000 == 1:
        return convert_to_word(x // 1000) + num_word_map[1000]
    if 0 < x//100 < 10:
        if x % 100:
            return convert_to_word(x//100) + num_word_map[100] + convert_to_word(x % 100, 'and')
        else:
            return convert_to_word(x//100) + num_word_map[100]
    elif 2 <= x//10 < 10:
        return spacing + num_word_map[x - (x % 10)] + convert_to_word(x % 10)
    # 0 included to prevent TypeError on a value like 80
    elif 0 <= x < 20:
        return spacing + num_word_map[x]
    else:
        "Value out of bounds"


def count_letters(limit=1000):
    """
    Counts the number of letters in a word corresponding to a number from one to limit

    Parameters
    ----------
    limit : upper limit of numbers to convert and count to

    Returns
    -------
    None

    """
    count = 0
    for val in range(1, limit + 1):
        s = convert_to_word(val)
        print(s)
        count += len(s)
    print(count)


if __name__ == '__main__':
    count_letters()
\$\endgroup\$
9
\$\begingroup\$

Nice use of """docstrings""".


This code is useless:

else:
    "Value out of bounds"

If the else clause is reached, the string statement is executed, with no effect. Just like executing a docstring statement has no effect.

You want:

else:
    raise ValueError("Value of out bounds")

The function convert_to_word() returns strange values like "sevenhundredandtwentyfive", which is only useful for counting letters, and nothing else. A more flexible return might be ["seven", "hundred", "and", "twenty", "five"], allowing the caller to count words, or letters ... or ... join the words with spaces.


Several recursive calls are unnecessary.

convert_to_word(x//100) could simply be num_word_map[x // 100], and convert_to_word(x % 10) could be num_word_map[x % 10].


Comparison of a floating point value and an integer for equality is dangerous x / 1000 == 1. Simply use x == 1000.


For this problem, instead of storing the words in num_word_map, you could simply store the lengths of the words.


Your """docstring""" usage is good. You describe what the function does, what the parameters are, and what the return value is. Someone wishing to use your functions would only need to type help(convert_to_word) and they should have all the information required to use the function.

There is no formal "right way" to format the docstrings, but there are some tools which expect things a certain way.

For instance

"""
... omitted ...

NOTE: Do not count spaces or hyphens. For example, 342 (three hundred and forty-two) contains
23 letters and 115 (one hundred and fifteen) contains 20 letters. The use of "and" when writing
out numbers is in compliance with British usage.
Answer = 21124
"""

This docstring contains 2 examples with answers, and one answer to a large question. You can embed examples in a """docstring""", and it can become a built-in test for your module.

"""
... omitted ...

Answer = 21124

NOTE: Do not count spaces or hyphens.  The use of "and" when writing
out numbers is in compliance with British usage.  For example:

>>> len(convert_to_word(342))     # three hundred and forty-two
23

>>> len(convert_to_word(115))     # one hundred and fifteen
20
"""

Starting at the >>>, the text look like lines straight out of what you might see if you typed commands in a Python REPL. The lines beginning with >>> are the commands, and the lines below that are the command output. The doctest module can read the docstrings in a module, find lines formatted this way, execute the commands, and verify the output matches the expected output. You might run this as:

python3 -m doctest numbers-letters.py

The output of the above command should be nothing, because the tests would pass. Let's add one more test in the doc string:

"""
... omitted ...

>>> len(convert_to_word(342))     # three hundred and forty-two
23

>>> len(convert_to_word(115))     # one hundred and fifteen
20

>>> convert_to_word(115)
'one hundred and fifteen'
"""

Now running the doctest will produce output due to the failing test:

python3.8 -m doctest number-letters.py 
**********************************************************************
File "/Users/aneufeld/Documents/Stack Overflow/number-letters.py", line 23, in number-letters
Failed example:
    convert_to_word(115)
Expected:
    'one hundred and fifteen'
Got:
    'onehundredandfifteen'
**********************************************************************
1 items had failures:
   1 of   3 in number-letters
***Test Failed*** 1 failures.

The output doesn't match because the test output contains spaces, which the function doesn't actually produce.

Other tools exist which read the docstrings, such as the Sphinx documentation generator. It has formatting requirements; read the docs for details.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the feedback. It's really helpful! \$\endgroup\$ – engineer.larry Dec 27 '19 at 20:27

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.