HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMajor

Program to print a Python list as "a, b, and c"

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
programprintpythonandlist

Problem

I am a beginner (with zero Python experience) working through the book Automate the boring stuff with Python. At the end of Chapter 4, the author provides a review problem.

I was supposed to write a function that takes a list value 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. Ex: 'apples, bananas, tofu, and hot dogs'. My function should be able to work with any list value passed to it.

I pulled together working code for the problem, but I cannot help but feel like there are concepts I am not quite understanding. My solution is long, compared to the answers for a related Stack Overflow question. I feel that there must be redundant code in my answer. Could I bother someone to please point out obvious logical errors or areas where I use redundant code?

That said, I did include an error checking function (slightly outside the scope of the question) since we briefly learned about try and except usage in the previous chapter, and because I don't like the idea that a capital letter at the beginning of the input is able to break the program.

```
# defining possible lists
lists = ['cars','favorite foods','family members']
x = ['Hyundai','Acura','Honda','Suzuki','Ford']
y = ['apples','bananas','tofu','hot dogs']
z = ['Mother','Father','Dog','Cat','Grandmother','Grandfather','Cousins','Host family']

# defining functions
def listselection():
print('The following lists exist within the system. Which would you like to see?')
print(lists)

def errorchecking():
global option

while True:
try:
option = str(input())
except:
print('Please type letters, not numbers.')
continue

if option == 'cars':
break
elif option == 'favorite foods':
break
elif option == 'family members':
break
else:
print('Please re-enter your selection')

return option

de

Solution

# defining possible lists


Don't write comments that describe what the code does; we can see the code, we know what it's doing. If you ever need comments, it's where it's otherwise unclear why the code's doing it.

lists = ['cars','favorite foods','family members']


Per the style guide, you should put spaces after the commas:

lists = ['cars', 'favorite foods', 'family members']


x = ['Hyundai','Acura','Honda','Suzuki','Ford']


x is a terrible name for this list (see also: y, z). Names should be more descriptive:

cars = ['Hyundai', 'Acura', 'Honda', 'Suzuki', 'Ford']


But given that lists is supposed to refer to the three lists below it, why not use a dictionary instead:

lists = {
    'cars': ['Hyundai', 'Acura', 'Honda', 'Suzuki', 'Ford'],
    ...
}


def listselection():
    print('The following lists exist within the system. Which would you like to see?')
    print(lists)


The style guide recommends snake_case names for functions. It's helpful to include a docstring when you define a function. Given the above switch to a dictionary, we need to change the second line slightly:

def list_selection():
    """Show the user the selection of available lists."""
    print('The following lists exist within the system. Which would you like to see?')
    print(list(lists))


Note that by default you get a dictionary's keys when you iterate over it, so list(lists) == ['cars', ...].

def errorchecking():    
    global option

    while True:
        try:
            option = str(input())
        except:
            print('Please type letters, not numbers.')
            continue

        if option == 'cars':
            break
        elif option == 'favorite foods':
            break
        elif option == 'family members':
            break
        else:
            print('Please re-enter your selection')

    return option


Using a while True loop to take and validate input is a good idea, but there are a few problems here:

-
Why do you use global option? You return option at the end of the function, which is the way you ought to provide it to other functions. Using global state is a bad pattern, get out of the habit of doing it.

-
Your error checking doesn't actually do anything. The result of input(...) is always a string, str(input(...)) will never cause an exception. Also your error checking is too broad; bare except is a bad idea, you should be specific about the kind of errors you expect and allow unexpected exceptions to get raised up. "Errors should never pass silently. Unless explicitly silenced."

-
This part:

if option == 'cars':
    break
elif option == 'favorite foods':
    break
elif option == 'family members':
    break
else:
    print('Please re-enter your selection')


is not very DRY (Don't Repeat Yourself); we already have those options, they're the keys to our dictionary (or the values of your list in the original). Therefore we could write:

if option in lists:
    break
else:
    print('Please re-enter your selection')


With all of the above changes the function is now a lot more concise:

def error_checking():
    """Take valid input from the user."""
    while True:
        option = str(input())
        if option in lists:
            return option  # this will also end the loop
        print('Please re-enter your selection')


Now it's clear that error_checking doesn't really describe what this function does; that's only part of its job, but the name should tell you how to use it. I would call it something like get_valid_input.

def listconcatenation():
    global spam
    global listitem

    listitem = 'Here are the items in my list: '

    if option == 'cars':
        spam = x
    elif option == 'favorite foods':
        spam = y
    elif option == 'family members':
        spam = z

    for i in spam:
        if spam.index(i) < (len(spam)-1):
            listitem = listitem + i + ', '
        else:
            listitem = listitem + 'and ' + i + '.'

    return listitem


Same things again: put underscores in the names; remove the global state (make option a parameter instead, you already return listitem). But there are three more specific improvements:

-
The dictionary we introduced above means you can go from this:

if option == 'cars':
    spam = x
elif option == 'favorite foods':
    spam = y
elif option == 'family members':
    spam = z


to this:

item_list = lists[option]  # note also a more descriptive name


-
That said, why do this inside list_concatenation? That makes that function very tightly coupled to the place where you're currently using it, it's not reusable at all. Instead, why not pass in the selected list, rather than the option name, as a parameter?

Similarly including things like 'Here are the items in my list: ' and the trailing full stop '.' within the function reduce its reusability.

-
In terms of l

Code Snippets

# defining possible lists
lists = ['cars','favorite foods','family members']
lists = ['cars', 'favorite foods', 'family members']
x = ['Hyundai','Acura','Honda','Suzuki','Ford']
cars = ['Hyundai', 'Acura', 'Honda', 'Suzuki', 'Ford']

Context

StackExchange Code Review Q#159081, answer score: 23

Revisions (0)

No revisions yet.