16
\$\begingroup\$

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

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


# run program
listselection()
errorchecking()
listconcatenation()
print(listitem)
\$\endgroup\$
24
\$\begingroup\$
# 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 looping in Python:

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

    The i would usually suggest an index, perhaps item would be a better name. If you want the index, rather than using spam.index(item) you can enumerate over the list:

    for index, item in enumerate(item_list):
        if index < (len(item_list) - 1):
    

    This is more efficient as you're only passing over the list once, rather than every time you need the index. Note the additional whitespace, again per the style guide.


With all of the above, we have to change the invocation of the functions:

list_selection()
item_list = lists[take_valid_input()]
result = list_concatenation(item_list)
print('Here are the items in my list: {}.'.format(result))

Now the data is moving through the functions in a logical way, rather than being updates to the global module state, and list_concatenation is a very generic, reusable function.

I might also be inclined to move list_selection inside take_valid_input, or even write a more generic function for user input that:

  • Takes a dictionary of options;
  • Displays the option names to the user;
  • Loops until they've entered one of the option names; and
  • Returns the one they've entered.
\$\endgroup\$
  • 1
    \$\begingroup\$ Good what you said about x, y and z. I would like to add that lists is not a good name either because it is commonly advised not to use a container's type as a name \$\endgroup\$ – Billal Begueradj Mar 28 '17 at 17:33
  • 1
    \$\begingroup\$ @BillalBEGUERADJ mainly because if you e.g. call a list list it shadows the built-in; lists doesn't. Also it contains lists! However there is probably a better name. \$\endgroup\$ – jonrsharpe Mar 28 '17 at 17:48
  • \$\begingroup\$ I think it could still arguably be considered Hungarian notation, but it doesn't seem that bad. \$\endgroup\$ – Dinerdo Mar 28 '17 at 18:14
  • 2
    \$\begingroup\$ @jonrsharpe Thank you for the tailored feedback! I truly appreciate your time spent typing this out. 1) I wasn't aware of the style guide. I'll review it. 2) I wasn't aware of dictionaries. I see it's covered next in the book so what you've pointed out helps a lot with context. 3) At first, I tried to define all global variables at the beginning of the program instead of inside functions. It didn't work. I wasn't clear that return would work on local variables so thank you for pointing this out. You've given me a lot to think about. \$\endgroup\$ – JAG Mar 28 '17 at 22:45
  • \$\begingroup\$ @JAG this seems to confuse a few people about global - see e.g. stackoverflow.com/q/4693120/3001761, stackoverflow.com/q/13881395/3001761. You don't use the keyword to declare a variable as global, you use it to tell a scope (e.g. a function) to use the global version of that name. \$\endgroup\$ – jonrsharpe Mar 29 '17 at 10:48
14
\$\begingroup\$

To expand on jonrsharpe's excellent answer, here is how you can make the function that does the actual work, printing the list comma separated with an "and" before the last item, more pythonic:

def list_concatenation(item_list):
    # Join all items except the last one with a comma between them
    out = ", ".join(item_list[:-1])
    # Add the last element, separated by "and" and a final "."
    return "{} and {}.".format(out, item_list[-1])

This uses str.join to join all but the last element with commas and str.format to assemble the final string.

I added some comments to explain the two steps, even though they are not very good comments (since they explain what the code does, instead of explaining why).

I could also have used itertools.islice:

from itertools import islice

def list_concatenation(item_list):
    out = ", ".join(islice(item_list, -1))
    ...

This creates a generator of the slice, which is lower in memory. But, as @jonrsharpe pointed out in the comments, this is slightly slower, because the first thing join does is consuming the iterator into a list to see how much memory it needs to allocate. Have a look at this link for more information on why this is slower.

After seeing the proposed unittests in @TobySpeight's answer, this is how you could make this function pass them all:

def list_concatenation(item_list):
    if not item_list:
        return ""
    elif len(item_list) == 1:
        return "{}.".format(item_list[0])
    # Join all items except the last one with a comma between them
    out = ", ".join(map(str, item_list[:-1]))
    # Add the last element, separated by "and" and a final "."
    return "{} and {}.".format(out, item_list[-1])

It can now even take a list of something different than strings, as suggested in @Artichoke's answer.

\$\endgroup\$
  • 1
    \$\begingroup\$ str.join will immediately consume that iterator into a list, I'm not sure you actually gain anything with itertools there. See e.g. stackoverflow.com/q/32462194/3001761 \$\endgroup\$ – jonrsharpe Mar 28 '17 at 11:24
  • \$\begingroup\$ As to why, it's because PyUnicode_Join calls PySequence_Fast, which turns everything but a list or tuple into a list. \$\endgroup\$ – jonrsharpe Mar 28 '17 at 11:33
  • \$\begingroup\$ @jonrsharpe Good find! I actually had this argument before, where my argument was that if join ever gets a better implementation, you immediately take advantage of it. But that relied on the assumption that it is just not faster than passing a list, I did not know/consider that it could be measurably slower! \$\endgroup\$ – Graipher Mar 28 '17 at 11:44
  • \$\begingroup\$ @jonrsharpe See the updated answer, where I included that information \$\endgroup\$ – Graipher Mar 28 '17 at 11:49
  • 2
    \$\begingroup\$ Nice, thanks. Also, purely from a domain perspective, you're probably not going to be generating 'one, two, ... , nine hundred and ninety-nine million nine hundred and ninety-nine thousand nine hundred and ninety-nine and a billion'! \$\endgroup\$ – jonrsharpe Mar 28 '17 at 12:05
11
\$\begingroup\$

Others have commented on your code; I'll concentrate on your test sets.

x = ['Hyundai','Acura','Honda','Suzuki','Ford']
y = ['apples','bananas','tofu','hot dogs']
z = ['Mother','Father','Dog','Cat','Grandmother','Grandfather','Cousins','Host family']

Whilst these are fine, they are actually quite similar, and therefore don't have any more value than a single test list. I think that the input to your formatter may be

  • an empty list
  • a list containing a single element
  • a list containing exactly two elements
  • a list containing three or more elements

I recommend you make one example of each of the above, and test them against expected output:

import unittest
class TestListFormat(unittest.TestCase):
    def test(self):
        self.assertEqual(list_concat([]), "")
        self.assertEqual(list_concat(["one"]), "one")
        self.assertEqual(list_concat(["one", "two"]), "one and two")
        self.assertEqual(list_concat(["one", "two", "three"]), "one, two, and three")

Bonus exercise

Modify your code to make the use of an Oxford comma configurable.

Harder exercise

Modify your code to work in languages other than English

\$\endgroup\$
  • \$\begingroup\$ yay, and my proposed function only fails on two of them :) \$\endgroup\$ – Graipher Mar 28 '17 at 15:07
  • \$\begingroup\$ +1 for this - my first thought when I saw the code too. I'd rather have ugly code that works than pretty code that doesn't! \$\endgroup\$ – xorsyst Mar 29 '17 at 11:51
6
\$\begingroup\$

Some excellent answers here, but just to show how powerful Python can be, consider this one line solution (using a few tricks):

    def list_print(list_in):
        print ', '.join([{len(list_in)-1:'and '}.get(index, '')+str(item) for index, item in enumerate(list_in)])

Please note that this is not in line with Python coding principles, which prefer longer code that is clearer to read, but one of the things I love about the language, coming from a C/C++ background, is how much you can do with just a little code.

Also, note that the results in English make sense with lists of 3 or more. With a list of 2, you will get 'first item, and second item' which is probably not what you want in that case

The tricks:

  1. using the string join() method to combine the elements of a list
  2. using a dict to differentiate in-line between several cases (namely, the last item on the list or not the last item)
  3. using the dict get() method to elegantly cover all cases except the last one
  4. using list comprehension to create a new list in-place
  5. using the enumerate() method to give easy access to both the index and the content of an iterable

Again, this wouldn't be the preferred solution, but it showcases some of the other functionality. In practice, I would prefer Graipher's solution, which as it happens can also be reduced to a single line, if one is so inclined, and also doesn't suffer from the comma limitation. The only thing I would change on his code is to explicitly typecast the list elements as strings, just in case someone passes in an int or float (or whatever else!) as one of the elements. As written, such a situation would cause a TypeError exception because you cannot concatenate str and int objects.

\$\endgroup\$
  • \$\begingroup\$ +1 for the comment on types. It is indeed one of the major limitations of my code. How nice would it be if we had a hybrid between str.format that automatically calls str on its arguments (or their __str__ methods) and str.join to join the elements. Without it we have to use ", ".join(map(str, iterable))... \$\endgroup\$ – Graipher Mar 28 '17 at 18:55
2
\$\begingroup\$

I believe there are few missings in the solutions. When we are suggesting him to remove global variables or else the global variables must be declared at the top. The better part is to avoid it and pass them as an argument or make nested functions or a method and a function, whatsoever.

Either the concat_list function may be a class with one arg lists and error_checking/input_option may be the part of concat_list.

otherwise, function concat_list may have two arguments list and option. Correct me, if I am wrong.

New contributor
S Habeeb Ullah is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\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.