Return values
def list_to_string(some_list):
new_string = ''
if not some_list:
return ''
...
Why are you returning ''
here? You could return new_string
instead, since you've initialized it.
In fact, the last statement in the function is return new_string
. Why not make that the only place you return from the function?
def list_to_string(some_list):
new_string = ''
if some_list:
if len(some_list) == 1:
new_string = str(some_list[0])
else:
for i in range(len(some_list) - 2):
new_string += str(some_list[i]) + ', '
new_string += str(some_list[len(some_list) - 2]) + ' and ' + str(some_list[len(some_list) - 1])
return new_string
Loop over values, not indices
Python is slow at indexing. Not terribly slow, but slow. In general, you want to avoid for i in range(len(container)):
type loops, especially when you only use i
in the expression container[i]
inside the loop.
So this code:
for i in range(len(some_list) - 2):
new_string += str(some_list[i]) + ', '
can become
for term in some_list[:-2]:
new_string += str(term) + ', '
Too many str calls
You are calling str(...)
way too many times. You need to call it once for each element of some_list
, which you are doing, but you've written the call to str(...)
four times, which is three times too many in my opinion. Better would be to convert all the terms to strings once, at the start.
some_list = [str(term) for term in some_list]
Now you can safely rely on the fact that all elements of the list are already strings.
Last & Second Last elements
some_list[len(some_list) - 1]
and some_list[len(some_list) - 2]
are clumsy ways of accessing the last and second last elements. Python allows negative indexing, which returns elements counting from the end of the list. some_list[-1]
is the last element and some_list[-2]
is the second last.
One element and last element
You've got two special cases. The one element case, which you just return directly, and the last element of a list of multiple items, which is handled differently.
Recognizing that a single element is the last element of a list of one element allows you to eliminate one special case.
def list_to_string(some_list):
new_string = ''
if some_list:
some_list = [str(term) for term in some_list]
if len(some_list) > 1:
for term in some_list[:-2]:
new_string += term + ', '
new_string += some_list[-2] + ' and '
new_string += some_list[-1]
return new_string
Type-hints, doc-strings, and testing oh my!
You might want to add some type-hints, """docstrings"""
and doctests to help your users understand how to use/call your function.
import doctest
def list_to_string(some_list: list) -> str:
"""
Convert a list of objects into a string of comma separated items.
The last two items are separated with ' and ' instead of a comma.
>>> list_to_string([])
''
>>> list_to_string(['apples'])
'apples'
>>> list_to_string(['apples', 'bananas'])
'apples and bananas'
>>> list_to_string(['apples', 'bananas', 'tofu', 'cats'])
'apples, bananas, tofu, and cats'
"""
new_string = ''
if some_list:
some_list = [str(term) for term in some_list]
if len(some_list) > 1:
for term in some_list[:-2]:
new_string += term + ', '
new_string += some_list[-2] + ' and '
new_string += some_list[-1]
return new_string
if __name__ == '__main__':
doctest.testmod()
The some_list: list
tells prospective callers that the function expects a list for the first (and only) argument. The -> str
tells prospective callers the function returns a string.
The """docstring"""
is help text which will be displayed if the user types help(list_to_string)
.
Finally, the lines starting with >>>
in the docstring are found by the doctest
module, and executed and compared to the following lines to verify the function operates as expected. Here we see:
**********************************************************************
File "/Users/aneufeld/Documents/Stack Exchange/Code Review/comma.py", line 17, in __main__.list_to_string
Failed example:
list_to_string(['apples', 'bananas', 'tofu', 'cats'])
Expected:
'apples, bananas, tofu, and cats'
Got:
'apples, bananas, tofu and cats'
**********************************************************************
1 items had failures:
1 of 4 in __main__.list_to_string
***Test Failed*** 1 failures.
... which tells us that something is amiss. Your problem text says your function should return one thing, but your code returns another!