1
\$\begingroup\$
spam = ['apples','dogs','tofu']

try:
    def comma(items):
        if range(len(items) >= 2):
            for i in range(len(items) - 2):
                print(items[i], end=', ')

            if range(len(items) > 2):
                print(items[-2] + ', and ' + items[-1])
            elif range(len(items) <= 2):
                print(items[-2] + ' and ' + items[-1])

        else:
            print(items[0])


    comma(spam)

except IndexError:
    print('Index Error: please enter a valid list.')
\$\endgroup\$
2
  • 8
    \$\begingroup\$ You forgot to describe in plain English what the code is supposed to do. \$\endgroup\$ Jun 22 at 23:17
  • \$\begingroup\$ I'd suggest including the problem description from automatetheboringstuff.com/chapter4 in your post to make it clear to readers what your code is trying to do. \$\endgroup\$
    – Setris
    Jun 23 at 13:22

2 Answers 2

3
\$\begingroup\$

Don't use try-except blocks casually or excessively. You don't need them in the current case. And when exception handling is truly needed, you should restrict its use to the narrowest possible scope -- the specific lines where the error can occur.

You code will fail if given an empty list. Specifically, this line: print(items[0]).

Whenever feasible, write functions that return data rather than causing side effects. Printing is one example of a side effect, and it is better to keep that kind of behavior separate from the logical complexity of the program. In the current situation, our primary logic is joining items into a comma-phrase. That logic should be done in a data-oriented function: take the items as input and return a string as output. Print elsewhere -- for example, in a main() function that orchestrates the program by subjecting the comma-phrase logic to various tests.

Speaking of tests, get in the habit of writing them. As illustrated below. Even better would be to learn how to use one of the Python testing frameworks, such as pytest.

Breaking down a problem. It's easy to overcomplicate a problem like this if you jump too quickly into coding. Step back and analyze the cases as shown in the table below. If we have 3 or more items, we need to do some comma-joining. But in all other cases, we can just return an and-joined string.

items           desired output
------------------------------
[]              ''
[A]             'A'
[A, B]          'A and B'
[A, B, C]       'A, B, and C'
[A, B, C, D]    'A, B, C, and D'

Don't forget about list slicing. For the comma-joining logic, we can grab all items other than the last one via a slice: items[0:-1].

def main():
    # Some test data.
    XS = ['A', 'B', 'C', 'D']
    TESTS = (
        ([], ''),
        (XS[0:1], 'A'),
        (XS[0:2], 'A and B'),
        (XS[0:3], 'A, B, and C'),
        (XS, 'A, B, C, and D'),
    )
    # Exercise our function.
    for items, exp in TESTS:
        got = comma_phrase(items)
        if got == exp:
            print('ok')
        else:
            print(items, (got, exp))

def comma_phrase(items):
    if len(items) < 3:
        return ' and '.join(items)
    else:
        return ', '.join(items[0:-1]) + ', and ' + items[-1]

if __name__ == '__main__':
    main()
\$\endgroup\$
2
\$\begingroup\$
        if range(len(items) >= 2):
            ...
            if range(len(items) > 2):
                ...
            elif range(len(items) <= 2):
                ...

What are you doing here? This code is very obfuscated, bordering on outright invalid.

>>> help(range)
Help on class range in module builtins:

class range(object)
 |  range(stop) -> range object
 |  range(start, stop[, step]) -> range object
 |
 |  Return an object that produces a sequence of integers from start (inclusive)
 |  to stop (exclusive) by step. ...
 |  ...

As you can see from the help, range() expects between 1 and 3 arguments, which are expected to be integers. You are passing in the result of boolean expressions, so either True or False. In integer context, True and False evaluate to 1 and 0 respectively.

>>> range(True)
range(0, 1)
>>> range(False)
range(0, 0)

Now, these range object are then used in boolean contexts in the if and elif conditions. Non-empty ranges are True and empty ranges are False ...

>>> bool(range(0, 1))
True
>>> bool(range(0, 0))
False

... so you've gone full circle bool(range(True)) evaluates to True and bool(range(False)) evaluates to False. The calls to range(...) do nothing for you. The statements could simple have been:

        if len(items) >= 2:
            ...
            if len(items) > 2:
                ...
            elif len(items) <= 2:
                ...

Of course, len() will always return a nice safe integer value (never NaN), so if len(items) > 2 is not true, then len(items) <= 2 will be true, and you could use a simple else clause.

        if len(items) >= 2:
            ...
            if len(items) > 2:
                ...
            else:
                ...
\$\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.