10
\$\begingroup\$

This is a practice from "Automate the boring stuff with Python".

My code works, but after checking out some other solutions on the Internet my solution looks more basic, and I can't tell if that's a good thing or not and if I got the right lessons from this chapter.

This is the code:

def items(someList):
    stringEnd = str(someList[-1])
    del someList[-1]
    str1 = ', '.join(someList)
    new_string = ''
    new_string = str1 + ' and' + ' ' + stringEnd
    return(new_string)

spam = ['apples', 'kittens', 'tofu', 'puppies']
print(items(spam))

Any feedback will be really appreciated!

Here is what the book required:

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.

\$\endgroup\$
  • 4
    \$\begingroup\$ You might be interested in this question. This code sounded familiar to me for some reason :) \$\endgroup\$ – Ludisposed Nov 7 '17 at 10:39
  • 2
    \$\begingroup\$ A bit of context about what this code is supposed to do would help us :) \$\endgroup\$ – Grajdeanu Alex. Nov 7 '17 at 11:02
  • \$\begingroup\$ @MrGrj Added more info to the OP! :) \$\endgroup\$ – Sloppy Coder Nov 7 '17 at 11:07
  • 2
    \$\begingroup\$ I suspect there's no comma between tofu and cats, is it? \$\endgroup\$ – Grajdeanu Alex. Nov 7 '17 at 11:09
  • 1
    \$\begingroup\$ @MrGrj Definitely not. \$\endgroup\$ – Sloppy Coder Nov 7 '17 at 11:11
13
\$\begingroup\$

The problem is pretty straightforward, and your solution is as well. I have only a couple of small comments:

  1. The pop method in Python removes and returns the last item in a list, which can replace two lines.
  2. You don't have to new_string = '' when you set new_string on the next line. As a result, things can be shortened a bit.
  3. Personally, since str1 is only used once, I would personally just do it inline and not save it to a variable. The only time I save something to a variable, if it is only used once, is to shorten an otherwise-long line.
  4. Concatenating two hard-coded strings should be put in one operation (i.e. ' and' + ' ' => ' and '
  5. Guards! They're the best way to check input before executing.

My recommended version:

def items(some_list):
    if not some_list:
        return ''

    string_end = some_list.pop()
    if not some_list:
        return string_end

    and_display = ' and ' if len(some_list) == 1 else ', and '
    new_string = ', '.join(some_list) + and_display + string_end
    return new_string

print(items(['apples', 'kittens', 'tofu', 'puppies']))

This works fine as long as you are working with a list of strings. It will crash if you pass in an int or something that can't auto-convert to a string. That may or may not be a problem, depending on your perspective. Trying to make your code auto-convert things to strings, if they aren't strings, can be a very error-prone process. As a result, I think it is perfectly fine to write this the easy way, and let it be the developer's problem if it crashes due to non-string arguments.

Docstrings are usually helpful for this kind of thing.

Note on pass-by-reference

Hat tip to TheEspinosa: Python uses a pass-by-reference style for its arguments. As a result, modifying arguments (which is especially easy for lists, as is the case here) can result in unintentional side-effects. The use of pop (or del in your original code) results in the last entry being removed from the list, not just inside the function, but also in the variable passed in to the function. For your example usage, which calls this function with a constant value, this behavior doesn't matter. But it may as part of a larger application. There are three ways to handle this:

  1. Adjust your algorithm so you don't make any changes to any arguments being passed in (@Richard Nemann's answer works that way)
  2. Copy the list before using it inside your function. A simple shorthand to copy a list is: new_copy = old_copy[:]. There is also a copy module.
  3. Ignore it and let it be the caller's problem.

I use all three solutions depending on the circumstances or my mood. A copy operation obviously involves some overhead, but unless you know that performance is a problem, I would still do a copy if the algorithm that avoids the need for the copy is harder to understand.

To be clear about the problem the current code can potentially introduce, try running my implementation (or yours) like this:

vals = ['apples', 'bananas', 'grapes']
items(vals)
print(len(vals))

If you run this code the last line will print 2, because in your calling scope the variable has been modified-by-reference: grapes is no longer in the list.

I said at the beginning that this was a straightforward problem. At this point in time though I think it's fair to say that the devil is in the details :)

\$\endgroup\$
  • 6
    \$\begingroup\$ Variable names should be snake_case instead of camelCase in python PEP8 \$\endgroup\$ – Ludisposed Nov 7 '17 at 11:22
  • 8
    \$\begingroup\$ Whenever easily possible, I would prefer a function that does not change state. So I would avoid list.pop() and del list[-1] \$\endgroup\$ – TheEspinosa Nov 7 '17 at 13:36
  • 4
    \$\begingroup\$ The question does ask for the Oxford comma. OP (and your answer) simply implemented the specification wrongly. That said, this is awkward if there are only two items in the list: even ardent users of the Oxford comma would omit it in the construct “apples, and bananas”. \$\endgroup\$ – Konrad Rudolph Nov 7 '17 at 14:53
  • 3
    \$\begingroup\$ -1 for basing whether you modifying the caller's data based on your mood. Modifying the caller's data is something that should be done very sparingly. In this case, you also make an assumption about the caller's type; if I pass in a generator or tuple, your code breaks. \$\endgroup\$ – jpmc26 Nov 8 '17 at 0:17
  • 2
    \$\begingroup\$ @KonradRudolph The Oxford comma only applies to lists with 3 or more elements by definition. “apples, and bananas” is not a use of the Oxford comma; it is simply incorrect punctuation. The book's problem statement does not make any explicit notes about what to do when there are only two elements, just as it makes no notes about a single element. The most reasonable assumptions would be to follow English rules or not support those inputs and throw a ValueError error. \$\endgroup\$ – jpmc26 Nov 8 '17 at 2:39
17
\$\begingroup\$

Your problem can be reduced to:

def list_english(items):
    """Returns a comma-separated string of the list's items,
    separating the last item with an 'and'.
    """
    if not items:
        return ''

    if len(items) == 1:
        return items[0]

    return '{}, and {}'.format(', '.join(items[:-1]), items[-1])

Note that variable and function names in python are to be named with underscore-separated lower-case according to PEP 8.
Also consider using docstrings to state, what a function's purpose is.

if __name__ == '__main__':
    print(list_english(['apples', 'kittens', 'tofu', 'puppies']))

Also enclose your script code in an if __name__ == '__main__': block to prevent it from being run if the module is solely imported.

Finally, consider your variable naming. Python has dynamic typing, so expressive names are especially important.

\$\endgroup\$
  • 1
    \$\begingroup\$ I might suggest inverting your first conditional. You've basically got all your code inside an if condition, which is always returned from, and a return '' at the end. Better to: if not items: return '' at the top and then un-indent what is the meat of your application. Basically, use a more standard guard clause. \$\endgroup\$ – Conor Mancone Nov 7 '17 at 14:27
  • 2
    \$\begingroup\$ You need a len(items) == 2 case. \$\endgroup\$ – Jared Goguen Nov 7 '17 at 18:13
  • 1
    \$\begingroup\$ list_english(['a', 'b']) -> a, and b, who would want this? \$\endgroup\$ – Jared Goguen Nov 7 '17 at 18:17
  • 1
    \$\begingroup\$ @RichardNeumann I think the main problem is that the book-question is unclear. Normally, an oxford comma is only relevant to lists of 3 or more: regardless of what "type" of comma you use, it is not standard practice to put a comma in a list of two. The distinction only comes with 3 or more. For oxford, include the comma. Otherwise, don't. However, the way the book-question is worded suggests that there should always be a comma, even for a list of two. As a result, your answer solves the problem, but Jared's is also correct in saying that this is never proper english. \$\endgroup\$ – Conor Mancone Nov 7 '17 at 20:39
  • 1
    \$\begingroup\$ Does this work if a pass a generator? Would len consume the elements? \$\endgroup\$ – jpmc26 Nov 8 '17 at 0:20

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.