12
\$\begingroup\$

Below is my code for the Comma Code problem from Chapter 4 of Automate the Boring Stuff with Python. Is there anything I can do to make it cleaner?

Comma Code

Say you have a list value like this:

spam = ['apples', 'bananas', 'tofu', 'cats']

Write a function that takes a list of values as an argument and returns a string with all the items separated by a comma and a space, with 'and' inserted before the last item. For example, passing the previous spam list to the function would return 'apples, bananas, tofu, and cats'. But your function should be able to work with any list passed to it.

The output of this program could look something like this: apples, bananas, tofu, and cats

import sys

spam = ['apples', 'bananas', 'tofu', 'cats']

def print_list(list):
    for item in list:
        if len(list) == 1:
           print(list[0]) 
        elif item != list[-1]:
            print(item + ', ', end='')
        else:
            print('and ' + list[-1])

    
print_list(spam)
\$\endgroup\$

5 Answers 5

16
\$\begingroup\$

You've got an odd bug with your code.

Given the 8th menu item from the greasy spoon café:

spam = ["Spam", "Spam", "Spam", "egg", "Spam"]

You will get:

and Spam
and Spam
and Spam
egg, and Spam

since you are not testing whether you are at the last item in your list, but rather if the current item is the same as the last item in the list.


Testing for a length 1 list should not be done inside the loop of all items; it should be done exactly once, outside the loop. Ie (but still with the above bug):

if len(list) == 1:    # Test outside of loop
   print(list[0]) 
else:
    for item in list:
        if item != list[-1]:
            print(item + ', ', end='')
        else:
            print('and ' + list[-1])

Want you really want to do is print all but the last item in the list one way, and the last item a different way:

# If there are more than 1 items in the list...
if len(list) > 1:

    # Print all but the last item, with a comma & space after each
    for item in list[:-1]:
        print(item + ', ', end='')

    # with "and " printed at the very end, just before ...
    print("and ", end='')

# print the last (or only) item in the list.
print(list[-1])

although this still assumes at least one item in the list.


Alternately, you could join() all but the last item, and then add ", and " along with the final item.

msg = list[-1]
if len(list) > 1:
    msg = ", ".join(list[:-1]) + ", and " + msg
print(msg)

You are doing import sys, but not using sys anywhere. You can remove the import.

\$\endgroup\$
6
  • \$\begingroup\$ Why is it better to test for a length of zero in the outside loop? Thanks for your help :) \$\endgroup\$ Sep 20, 2019 at 22:35
  • \$\begingroup\$ Also, just a question I have had for a while. Why does the iterator not have to be defined before being called? For example "item" in your loop? for item in list[:-1]: \$\endgroup\$ Sep 20, 2019 at 22:38
  • \$\begingroup\$ The test is more efficient outside the loop, because it is done only once, instead of dozens or possibly hundreds of times, if you are looping over dozens or hundreds of entries. \$\endgroup\$
    – AJNeufeld
    Sep 21, 2019 at 0:15
  • 1
    \$\begingroup\$ Python doesn’t declare any variables. You can assign x = 5 without previously declaring that x exists. The for statement just repeatedly assigns new values to the loop variable. It doesn’t care if the variable existed before the loop. It doesn’t even care if the variable is deleted inside the loop; it will just recreate it on the next iteration. \$\endgroup\$
    – AJNeufeld
    Sep 21, 2019 at 0:19
  • 1
    \$\begingroup\$ :trollface: " ,".join(list[::-1]).replace(',', 'dna ', 1)[::-1] \$\endgroup\$ Sep 21, 2019 at 11:58
7
\$\begingroup\$

The problem statement says (bold emphasis mine):

Write a function that takes a list of values as an argument and returns a string with all the items separated by a comma and a space, with 'and' inserted before the last item. For example, passing the previous spam list to the function would return 'apples, bananas, tofu, and cats'. But your function should be able to work with any list passed to it.

Your function doesn't return anything, it just prints. So, what do I do if I want to display a list of groceries on a website? Or store it in a database? Or write it to a file? Or send it as an email? Or pipe it into a text-to-speech synthesizer?

I'll have to write the exact same code over and over again!

Instead, you should separate computation from input/output. (Not just because the problem statement says so, but as a general rule.)

The immediate benefit is that you can easily test your code. If you print the result, in order to test the code, you would have to somehow intercept the standard output stream or re-direct the stream to a file and then read back and parse the file, or something similarly convoluted. If you separate constructing the string from printing the string, testing becomes much easier, you just write something like:

spam   = ['apples', 'bananas', 'tofu', 'cats']
result = 'apples, bananas, tofu, and cats'

assert result == commatize(spam)

See Emad's answer for an example of returning the result instead of printing it.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. Returning the result is one of the main things I need to remember to do. \$\endgroup\$ Sep 21, 2019 at 17:06
5
\$\begingroup\$

Unused import statement: that should be cleaned up.

import sys

Bad parameter name: def print_list(list): The keyword list() is a built-in function, you don't want your variable/parameter/function/method/class named like built-ins because later when you start relying on such keywords, this will create problems.

Example: (run this interactively)

>>> word = 'mouse'
>>> letters = list(word)
>>> letters
['m', 'o', 'u', 's', 'e']
>>> list = 5
>>> new_letters = list(word)

Troubles ...

Traceback (most recent call last):
File "<pyshell#3>", line 1, in <module>
list(word)
TypeError: 'int' object is not callable

I think the example is self-explanatory why you shouldn't use built-ins in your naming.

def print_list(list):

Docstrings: Python documentation strings (or docstrings) provide a convenient way of associating documentation with Python modules, functions, classes, and methods. ... A docstring is simply a multi-line string, that is not assigned to anything. It is specified in source code that is used to document a specific segment of code. You should include a docstring to your public functions indicating what they do and type hints if necessary.

This is a bit complicated

for item in list:
    if len(list) == 1:
       print(list[0]) 
    elif item != list[-1]:
        print(item + ', ', end='')
    else:
        print('and ' + list[-1])

The whole function can be simplified and written properly:

def separate(items: list):
    """Return a string of the words in items separated by a comma."""
    if len(items) > 1:
        return ', '.join(items[:-1]) + ', and ' + items[-1]
    if len(items) == 1:
        return items[0]
    if not items:
        raise ValueError('Empty list')

then use if __name__ == '__main__': guard at the end of your script which allows it to be imported by other modules without running the whole script.

if __name__ == '__main__':
    spam = ['apples', 'bananas', 'tofu', 'cats']
    print(separate(spam))
\$\endgroup\$
2
  • \$\begingroup\$ @EmadBoctor Thanks for your answer. How does the if name == 'main': work? \$\endgroup\$ Sep 20, 2019 at 22:29
  • 2
    \$\begingroup\$ You place the main guard at the end of your script and call your functions from there. And there are many cases where your module is imported by outside modules, for simplicity suppose you want to use that word separator from another script that let's say that organizes text or whatever, you will import your_script_name and use it accordingly. \$\endgroup\$
    – user203258
    Sep 20, 2019 at 22:44
1
\$\begingroup\$

In real-life, we'd do it like this:

def print_list(list):
    if len(list) > 1:
        return ', '.join(list[:-1]) + ', and ' + list[-1]
    elif len(list) == 1:
        return list[0]
    else:
        return ""

(However, it's an excellent learning exercise to do it without the .join builtin.)

When you use a negative number to access a list index, it tells Python to get the element from the back of the list. In this case, -1 means the last element of the list.

list[:-1] grabs a slice of all the elements of the list except for the last.

', '.join inserts ', ' between all the elements in the list

and

list[-1] grabs the last element of the list.

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

most people here are using things not thaught in the book such as join, join should not be used to solve this question. The answer could be:

if len(spam) == 1:
    print(spam[0])
else:
    for i in range(len(spam)-1):
        print(spam[i], end=', ')
print('and', spam[-1])

if your list is always >1 you can skip the if/else statement

\$\endgroup\$
1
  • \$\begingroup\$ This answer would be better if you explained why join should not be used to solve this question. \$\endgroup\$
    – pacmaninbw
    May 30 at 13:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.