13
\$\begingroup\$

I'm currently working through an exercise of Chapter 4 from Automate the Boring Stuff, which reads as follows:

"Say you have a list value like this: spam = ['apples', 'bananas', 'tofu', 'cats']

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. 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 value passed to it."

Since I'm an absolute beginner to Python (and programming in general), I wanted to get some advice on how to make this code cleaner. It works with any size list, but I've reviewed some other solutions on SO and it seems like there are a million ways to build this out. How can I make this more straightforward?

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

def commaCode(listVar):
    if len(listVar) == 0: # condition for an empty list
        print('There are no items in this list.') 

    if len(listVar) == 1: # condition for a list with 1 item
        return (str(listVar[0])+'.')

    if len(listVar) == 2: # condition for a list with 2 items
        return (str(listVar[0]) + ' and ' + str(listVar[1]) + '.')

    if len(listVar) >=3: # conditions for lists with 3+ items
        listDisp = ''
        for i in range(len(listVar[:-1])):
            listDisp = listDisp + str(listVar[i]) + ', '
        listDisp = listDisp + 'and ' + str(listVar[-1])
        return (listDisp)

commaCode(spam)
\$\endgroup\$
  • 2
    \$\begingroup\$ Ugh, that oxford comma... \$\endgroup\$ – RobIII Apr 10 '18 at 11:34
  • 8
    \$\begingroup\$ oxford commas are fine. I actually find myself tending towards them more often than not these days.... \$\endgroup\$ – Baldrickk Apr 10 '18 at 11:40
  • \$\begingroup\$ I'm not sure if you are interested at all in seeing other coding languages do this (I sometimes look at other languages so I can see how things are done), but I wrote a function in PHP that does this which is only a couple of lines of code. eval.in/986967 \$\endgroup\$ – GrumpyCrouton Apr 10 '18 at 13:00
  • \$\begingroup\$ As far as I can see, the task description does not require adding a dot at the end of a resulting string for a one- or two-items input list. Additionally, IMHO you don't need the separate if len(listVar) == 2 branch; the last if seems able to hande the 2-items case. \$\endgroup\$ – CiaPan Apr 11 '18 at 11:39
19
\$\begingroup\$
  1. You want to look at str.join
  2. by the last case, len(listVar) is run 4 times, which is very repetitive. Better to set it to a temporary variable, and check that each time.
  3. Python has some binary convenience operators for common things like appending. Instead of listDisp = listDisp + foo use listDisp += foo
  4. instead of calling str() on everything as you build the string, why not call it up front, so it can't be missed somewhere along the way. Something at the top like stringList = [ str(i) for i in listVar ] will remove the need for all those str() calls sprinkled throughout the code.
  5. Naming: listVar is a variable. It doesn't need to have 'var(iable)' in its name. How about 'inputList'? Names are important - they're messages to the future (you or whoever reads your code) about what you are/were trying to accomplish. Similar issue with the name commaCode.
  6. range(len(listVar[:-1])) is the same as range(len(listVar)-1), which is both easier to read, and can take advantage of point 2 and end up written as range(listVarLen-1) which is definitely clearer.

Overall though, for a beginner, not bad!

\$\endgroup\$
  • 7
    \$\begingroup\$ += is a binary operator, not a unary operator. \$\endgroup\$ – Mast Apr 10 '18 at 16:03
  • 1
    \$\begingroup\$ "Names are important - they're messages to the future" - well said! \$\endgroup\$ – CJ Dennis Apr 11 '18 at 4:59
8
\$\begingroup\$

@pjz had some very good points, so I'll skip those in my review.

  1. Both function names and variable names are written in snake_case in python.
  2. Instead of printing an error message, raise an appropriate exception.
  3. You append a period for input of length 1 and 2, but not for longer output.
  4. Using python list slicing you can actually remove some of your special cases.

Here's two suggested improvements, one easier and one more pythonic. I also included a short testing snippet to validate that the exception is raised.

def comma_code(input_list):
    if len(input_list) == 0:
        # Raise an exception rather than just printing the error
        raise ValueError('List cannot be empty')
    # Convert everything to string (could also raise an exception if not string)
    string_list = [str(i) for i in input_list]
    # Handle the trivial case
    if len(string_list) == 1:
        return string_list[0]

    # This could be even more pythonic, but it would lose its readability
    more_than_two_items = len(string_list) > 2
    first_part = ', '.join(string_list[:-2])
    optional_separator = ', ' * more_than_two_items
    last_part = ', and '.join(string_list[-2:])

    formatted_string = first_part + optional_separator + last_part
    return formatted_string     

def comma_code_pythonic(input_list):
    if len(input_list) == 0:
        raise ValueError('List cannot be empty')

    string_list = [str(i) for i in input_list]

    last_part = ', and '.join(string_list[-2:])
    first_part = string_list[:-2]

    formatted_string = ', '.join(first_part + [last_part])
    return formatted_string     


# Try to place non-global variables below the function
spam = ['apples', 'bananas', 'tofu', 'cats']

for i in range(5):
    try:
        print("comma_code:", comma_code(spam[:i]))
    except ValueError as e:
        print(repr(e))

for i in range(5):
    try:
        print("comma_code_pythonic:", comma_code_pythonic(spam[:i]))
    except ValueError as e:
        print(repr(e))
\$\endgroup\$
  • \$\begingroup\$ Great advice. Minor nitpick: why the exception? Concatenating an empty list is an entirely valid operation. After all, , '.join([]) in Python also works. Just return ''. \$\endgroup\$ – Konrad Rudolph Apr 10 '18 at 9:20
  • \$\begingroup\$ Thank you! I included the exception because there was a similar implementation in the original question, and it seemed as if the asker was not familiar with the concept of exceptions. If i was to solve the code myself, I wouldn't include it, but if I was to print an error message, I'd raise an exception. \$\endgroup\$ – maxb Apr 10 '18 at 9:47
3
\$\begingroup\$

This answer is less about the code you have produced, but more about how you could approach the problem if you were to do it again.

note, the python style guides promote snake_case over camelCase naming conventions, so I'll use these here.

"Say you have a list value like this: spam = ['apples', 'bananas', 'tofu', 'cats']

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.

What is the key functionality that needs to be done here?

Simply put, the main functionality of the code is to take the values in the list and convert this into a string.
This should immediately suggest that string.join() would be a good function for this.

As it turns out, we can join the strings with whatever sub-string we like, so by using

', '.join(list_of_strings)

we get the following transformation:

['apples', 'bananas', 'tofu', 'cats'] -> 'apples, bananas, tofu, cats'

That's almost the entire task complete! ('join()' handles the 0 and 1 length list arrays correctly, so you don't have to)
We just need to insert the 'and '.

A quick analysis of the problem shows that we only need the 'and ' when there are at least two items, so we write a modification to do just that.
We could just add 'and' in the penultimate location in the list, but we don't want to end up with ['x', 'y'] -> 'x, and, y' so the simple solution to this is to replace the final input in this case with 'and ' plus the input.

We can do this with one of the following lines:

#python 2+:
list_of_strings[-1] = 'and %s' % list_of_strings[-1]
#python 2.6+:
list_of_strings[-1] = 'and {}'.format(list_of_strings[-1])
#python 3.6+:
list_of_strings[-1] = f'and {list_of_strings[-1]}'

To ensure that this does not change the input (it may be reused elsewhere) we should make a copy of it first. The simple way is to create a new list from the original:

list_of_strings = list(input_list_of_strings)

Putting this all together, we get a fairly simple function as a result:

def comma_code(input_list_of_strings):
    list_of_strings = list(input_list_of_strings)
    if len(list_of_strings) > 1:
        list_of_strings[-1] = f'and {list_of_strings[-1]}'
    return ', '.join(list_of_strings)
\$\endgroup\$
  • \$\begingroup\$ list_of_strings[-1] = 'and {}'.format(list_of_strings[-1]) works in Py2.6+ \$\endgroup\$ – TemporalWolf Apr 10 '18 at 18:34
  • 1
    \$\begingroup\$ @TemporalWolf so it does. One of my machines at work is airgapped and only runs 2.4... I have become used to using % notation for that system. I use 3.6 everywhere else. \$\endgroup\$ – Baldrickk Apr 10 '18 at 22:43
2
\$\begingroup\$

To augment the other good answers already given, here's a short version that utilizes several of the already given ideas, as well as a couple additional ones:

>>> spam = ['apples', 'bananas', 'tofu', 'cats']
>>> comma_code = lambda l: ", ".join(l[:-1] + ['and ' + l[-1]])
>>> comma_code(spam)
'apples, bananas, tofu, and cats'

The lambda clause is a shorthand method for defining smaller functions, rather than using the full def ... syntax.

It also uses list slicing, and both string and list concatenation. As long as you know your input list is only strings, this should be fine, but if you're unsure of especially the last element in the list, one of the approaches using .format() could be integrated just as well.

Update: just realized this will fail for lists of less than two items. So that's another assumption you'd have to make on your input. Wrapping the above in a slightly larger function that dealt with the two corner cases would be relatively easy to do.

\$\endgroup\$
  • \$\begingroup\$ The last comma after tofu makes this answer incorrect. It should be: 'apples, bananas, tofu and cats' \$\endgroup\$ – victorantunes Apr 10 '18 at 19:57
  • 2
    \$\begingroup\$ @victorantunes Not by my reading of the original question, which explicitly lists the desired output as apples, bananas, tofu, and cats. \$\endgroup\$ – twalberg Apr 10 '18 at 20:02
  • \$\begingroup\$ My bad. I totally skipped that snippet and went straight into grammar mode. \$\endgroup\$ – victorantunes Apr 10 '18 at 20:16
  • \$\begingroup\$ not quite. Try comma_code(['cats']) it returns 'and cats' which should just be `cats' true, the question doesn't explicitly state this but your code also throws an IndexError with an empty input list which violates "your function should be able to work with any list value passed to it." \$\endgroup\$ – Baldrickk Apr 10 '18 at 22:52
  • 1
    \$\begingroup\$ @victorantunes Even in non-technical English grammar the comma after tofu is a well-accepted usage, the Oxford comma. \$\endgroup\$ – David K Apr 11 '18 at 6:30
0
\$\begingroup\$
>>> spam = ['apples', 'bananas', 'tofu', 'cats']
>>> print(', '.join(spam[:-2] + [spam[-2] + ' and ' + spam[-1]]))
apples, bananas, tofu and cats

The comma after tofu should not exist.

Steps:

  1. ', '.join(...) creates a comma-separated string of the values contained in spam
  2. spam[:-2] slices spam and creates a sublist from the first to the second-to-last item. Have a look at python's slice notation.
  3. [spam[-2] + ' and ' + spam[-1]] creates a list containing: spam's second-to-last item ('tofu'), the word 'and' and spam's last item ('cats'). Creating a list is necessary because python doesn't know how to concatenate a list + another type of object. We can only concatenates lists.

Edit: Oxford Comma is a thing, apparently. Well, hooray english.

>>> spam = ['apples', 'bananas', 'tofu', 'cats']
>>> print(', '.join(spam[:-1] + [' and ' + spam[-1]]))
apples, bananas, tofu, and cats
\$\endgroup\$
  • 3
    \$\begingroup\$ The comma after tofu is the Oxford comma. Whether to use it or not is an interesting question but moot here, because the specification includes it. \$\endgroup\$ – Graipher Apr 10 '18 at 20:53
  • \$\begingroup\$ @Graipher learning something new everyday, I suppose. English isn't my first language, so my first instinct was to yell 'incorrect!'. \$\endgroup\$ – victorantunes Apr 11 '18 at 12:53
  • \$\begingroup\$ I can relate, in my native language (German), this would also be a mistake \$\endgroup\$ – Graipher Apr 11 '18 at 12:55
0
\$\begingroup\$

There are some excellent pointers on the other answers, but I feel that they miss the most pythonic solution:

def comma_code(words):
    *head, final = words

    if head:
        final = f'and {final}'

    return ', '.join(*head, final)

This solution should be fairly clear to any reader, and to me reads as an encoding of the problem definition: given a non-empty list of words, join them with a comma. If there are more than one, add the word 'and' before the final word.

This code does not need to special-case the empty list as the unpacking operation will raise a ValueError for an empty input. You could catch this and return an empty string if this is the desired behaviour, or you could use varadiac arguments to clarify an interface like so:

def comma_code_args(first, *rest):
    *head, final = first, *rest

    if head:
        final = f'and {final}'

    return ', '.join(*head, final)

This would be called like so:

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

>>> comma_code(spam)
'apples, bananas, tofu, and cats'

>>> comma_code_args('apples', 'bananas', 'tofu', 'cats')
'apples, bananas, tofu, and cats'

>>> comma_code_args(*spam)
'apples, bananas, tofu, and cats'
\$\endgroup\$
0
\$\begingroup\$

This can be achieved with no loops and no joins, just two print statements:

def commalist(listname):
    print(*listname[:-1], sep = ', ',end=", "),
    print('and',listname[-1])

the end parameter in the first print will determine whether to use oxford comma.

\$\endgroup\$
  • \$\begingroup\$ Hey Mike, could you explain in little more detail why your solution is better. The goal of Code Review isn't to just "give better code" but to explain why so the OP can learn :) \$\endgroup\$ – IEatBagels Aug 22 at 15:31

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.