The problem is pretty straightforward, and your solution is as well. I have only a couple of small comments:
pop method in Python removes and returns the last item in a list, which can replace two lines.
- 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.
- 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.
- Concatenating two hard-coded strings should be put in one operation (i.e.
' and' + ' ' =>
' and '
- Guards! They're the best way to check input before executing.
My recommended version:
if not some_list:
string_end = some_list.pop()
if not some_list:
and_display = ' and ' if len(some_list) == 1 else ', and '
new_string = ', '.join(some_list) + and_display + string_end
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
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:
- Adjust your algorithm so you don't make any changes to any arguments being passed in (@Richard Nemann's answer works that way)
- 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.
- 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']
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 :)