Automate the boring stuff with python - Comma Code v2

Question

This is the second version of code reviewed earlier.

Comma Code

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.

def list_string(l_list):
    if not type(l_list) is list: # If not of type list throw's an error
        raise TypeError('Only lists are allowed')
    l_list = [str(word) for word in l_list] # Makes sure everything is a string.
    
    # Rules for Oxford comma
    if len(l_list) == 1:
        return f"'{l_list[0]}'"
    elif len(l_list) == 2:
        return f"'{l_list[0]}' and '{l_list[1]}'"
    else:
        return "'{}, and {}.'".format(', '.join(l_list[:-1]), (l_list[-1]))
            
        

def main() -> None:
    l_list = ['Apple', 'Lobster','Panda', 'Vis', 2, 3]
    output = list_string(l_list)
    print(output)
    

if __name__ == '__main__': # Program starts here, then main() get's called.
    main()

Spent 3 Hours on this one. Did look up some other code reviews; felt kinda cheating.

This time the program should do what the exercise asks.

Answer 1

Two hard things in computer science: cache invalidation, naming, off-by one bugs.

list_string() is not a good identifier. As you go on this journey of writing code, think hard about what to name things. Feel very free to come back and rename things as it becomes clearer what they really do. Also, I have no idea what the l in l_list denotes. The name main() is conventional and is perfectly good. I like your if __name__ guard.

Consider using this signature:

def oxford_comma(items: list[str]) -> str:

You don't have to include those optional type annotations. But notice that they can do some of the heavy lifting, precisely documenting what is expected, and then you have greater freedom to create an informative name which isn't burdened by a need to reveal data types.

---

if not type(l_list) is list:
# or equivalently...
if     type(l_list) is not list:

I suppose this literally fulfills the spec's contract. But a more pythonic approach, a looser more forgiving approach, would ask about isinstance(l_list, list).

That way a caller could define

class GroceryList(list):

which inherits from the base class of list. Yet we could still apply Oxford comma to it.

Also, please use $ ruff . or [flake8](https://pypi.org/project/flake8) to lint your code, and follow the advice they suggest.

---

would return 'apples, bananas, tofu, and cats'.

I fear you may have interpreted that instruction a bit too literally. I take the ' single quotes to indicate that a str value was returned, not that quotes shall be part of the result. Consider trimming the quotes.

Anyway, the comma .join() expression is very nice.

In the case of a zero-length list, [], you should probably return "" the empty string. Or perhaps raise ValueError("empty input") if you feel the caller violated the spec. Currently [] provokes an IndexError, which is not what you want.

        ... , (l_list[-1]))

Avoid decorating that last expression with extra ( ) parens, as they don't do anything.

---

Notice that you're working on a copy of the input, after you essentially assigned l_list = list(map(str, l_list)). So we are free to mutate it, without troubling the caller. Consider munging the final element, and then letting the comma .join() do most of the work:

        l_list[-1] = f'and {l_list[-1]}'

---

This code achieves its design goals.

I would be willing to delegate or accept maintenance tasks on it.

Answer 2

Hang in there; this is about to get a bit technical.

It's a natural, and overall quite reasonable, strategy to do as you've done and write a list comprehension such as

[str(word) for word in l_list]

But what does that actually do inside of the Python interpreter? Let's ask the disassembler:

import dis
def list_string(l_list):
    if not type(l_list) is list:  # If not of type list throw's an error
        raise TypeError('Only lists are allowed')
    l_list = [str(word) for word in l_list]  # Makes sure everything is a string.

    # Rules for Oxford comma
    if len(l_list) == 1:
        return f"'{l_list[0]}'"
    elif len(l_list) == 2:
        return f"'{l_list[0]}' and '{l_list[1]}'"
    else:
        return "'{}, and {}.'".format(', '.join(l_list[:-1]), (l_list[-1]))

dis.dis(list_string)

  6           0 LOAD_GLOBAL              0 (type)
              2 LOAD_FAST                0 (l_list)
              4 CALL_FUNCTION            1
              6 LOAD_GLOBAL              1 (list)
              8 IS_OP                    1
             10 POP_JUMP_IF_FALSE       10 (to 20)

  7          12 LOAD_GLOBAL              2 (TypeError)
             14 LOAD_CONST               1 ('Only lists are allowed')
             16 CALL_FUNCTION            1
             18 RAISE_VARARGS            1

  8     >>   20 LOAD_CONST               2 (<code object <listcomp> at 0x7f2c36c942f0, file "/home/gtoombs/petprojects/stackexchange/285474.py", line 8>)
             22 LOAD_CONST               3 ('list_string.<locals>.<listcomp>')
             24 MAKE_FUNCTION            0
             26 LOAD_FAST                0 (l_list)
             28 GET_ITER
             30 CALL_FUNCTION            1
             32 STORE_FAST               0 (l_list)

 11          34 LOAD_GLOBAL              3 (len)
             36 LOAD_FAST                0 (l_list)
             38 CALL_FUNCTION            1
             40 LOAD_CONST               4 (1)
             42 COMPARE_OP               2 (==)
             44 POP_JUMP_IF_FALSE       31 (to 62)

 12          46 LOAD_CONST               5 ("'")
             48 LOAD_FAST                0 (l_list)
             50 LOAD_CONST               6 (0)
             52 BINARY_SUBSCR
             54 FORMAT_VALUE             0
             56 LOAD_CONST               5 ("'")
             58 BUILD_STRING             3
             60 RETURN_VALUE

 13     >>   62 LOAD_GLOBAL              3 (len)
             64 LOAD_FAST                0 (l_list)
             66 CALL_FUNCTION            1
             68 LOAD_CONST               7 (2)
             70 COMPARE_OP               2 (==)
             72 POP_JUMP_IF_FALSE       50 (to 100)

 14          74 LOAD_CONST               5 ("'")
             76 LOAD_FAST                0 (l_list)
             78 LOAD_CONST               6 (0)
             80 BINARY_SUBSCR
             82 FORMAT_VALUE             0
             84 LOAD_CONST               8 ("' and '")
             86 LOAD_FAST                0 (l_list)
             88 LOAD_CONST               4 (1)
             90 BINARY_SUBSCR
             92 FORMAT_VALUE             0
             94 LOAD_CONST               5 ("'")
             96 BUILD_STRING             5
             98 RETURN_VALUE

 16     >>  100 LOAD_CONST               9 ("'{}, and {}.'")
            102 LOAD_METHOD              4 (format)
            104 LOAD_CONST              10 (', ')
            106 LOAD_METHOD              5 (join)
            108 LOAD_FAST                0 (l_list)
            110 LOAD_CONST               0 (None)
            112 LOAD_CONST              11 (-1)
            114 BUILD_SLICE              2
            116 BINARY_SUBSCR
            118 CALL_METHOD              1
            120 LOAD_FAST                0 (l_list)
            122 LOAD_CONST              11 (-1)
            124 BINARY_SUBSCR
            126 CALL_METHOD              2
            128 RETURN_VALUE

Disassembly of <code object <listcomp> at 0x7f2c36c942f0, file "/home/gtoombs/petprojects/stackexchange/285474.py", line 8>:
  8           0 BUILD_LIST               0
              2 LOAD_FAST                0 (.0)
        >>    4 FOR_ITER                 6 (to 18)
              6 STORE_FAST               1 (word)
              8 LOAD_GLOBAL              0 (str)
             10 LOAD_FAST                1 (word)
             12 CALL_FUNCTION            1
             14 LIST_APPEND              2
             16 JUMP_ABSOLUTE            2 (to 4)
        >>   18 RETURN_VALUE

code object <listcomp> is referenced, and then later defined, to be an inner function - even though you haven't explicitly asked for an inner function to be made. In fact, for the most common implementations of Python, this is how generators are created.

It's not the end of the world for a function like this to be created, but if a function needs to be created, we should replace it with our own explicit function that

• does everything we want it to without a pre-processing step: str()ing the items, yielding commas and yielding and;
• avoids the in-memory list that you have created, going straight from a generator to a join; and
• doing it all in a procedural style that is very easy to understand.

On top of that, this is a golden opportunity to write unit tests, so do that.

The generator approach can look like

from typing import Sequence, Any, Iterator


def comma_and_insertion(items: Sequence[Any]) -> Iterator[str]:
    match items:
        case (only,):
            yield str(only)
        case first, second:
            yield from (str(first), ' and ', str(second))
        case *others, second_last, last:
            for item in others:
                yield str(item)
                yield ', '
            yield from (str(second_last), ', and ', str(last))


def list_string(items: Sequence[Any]) -> str:
    """Format with Oxford commas and the word "and" """
    return ''.join(comma_and_insertion(items))


def test() -> None:
    assert list_string([]) == ''

    assert list_string(['foo']) == 'foo'

    assert list_string(['foo', 'bar']) == 'foo, and bar'

    assert list_string(
        ['Apple', 'Lobster', 'Panda', 'Vis', 2, 3]
    ) == 'Apple, Lobster, Panda, Vis, 2, and 3'


if __name__ == '__main__':
    test()

If we dis() our new inner function comma_and_insertion, we get something similar to your implicit listcomp (note the FOR_ITER) - but that takes on all of the work in one pass:

              0 GEN_START                0

  5           2 LOAD_FAST                0 (items)

  6           4 DUP_TOP
              6 MATCH_SEQUENCE
              8 POP_JUMP_IF_FALSE       19 (to 38)
             10 GET_LEN
             12 LOAD_CONST               1 (1)
             14 COMPARE_OP               2 (==)
             16 POP_JUMP_IF_FALSE       19 (to 38)
             18 UNPACK_SEQUENCE          1
             20 STORE_FAST               1 (only)
             22 POP_TOP

  7          24 LOAD_GLOBAL              0 (str)
             26 LOAD_FAST                1 (only)
             28 CALL_FUNCTION            1
             30 YIELD_VALUE
             32 POP_TOP
             34 LOAD_CONST               0 (None)
             36 RETURN_VALUE

  6     >>   38 POP_TOP

  8          40 DUP_TOP
             42 MATCH_SEQUENCE
             44 POP_JUMP_IF_FALSE       45 (to 90)
             46 GET_LEN
             48 LOAD_CONST               2 (2)
             50 COMPARE_OP               2 (==)
             52 POP_JUMP_IF_FALSE       45 (to 90)
             54 UNPACK_SEQUENCE          2
             56 STORE_FAST               2 (first)
             58 STORE_FAST               3 (second)
             60 POP_TOP

  9          62 LOAD_GLOBAL              0 (str)
             64 LOAD_FAST                2 (first)
             66 CALL_FUNCTION            1
             68 LOAD_CONST               3 (' and ')
             70 LOAD_GLOBAL              0 (str)
             72 LOAD_FAST                3 (second)
             74 CALL_FUNCTION            1
             76 BUILD_TUPLE              3
             78 GET_YIELD_FROM_ITER
             80 LOAD_CONST               0 (None)
             82 YIELD_FROM
             84 POP_TOP
             86 LOAD_CONST               0 (None)
             88 RETURN_VALUE

  8     >>   90 POP_TOP

 10          92 MATCH_SEQUENCE
             94 POP_JUMP_IF_FALSE       84 (to 168)
             96 GET_LEN
             98 LOAD_CONST               2 (2)
            100 COMPARE_OP               5 (>=)
            102 POP_JUMP_IF_FALSE       84 (to 168)
            104 EXTENDED_ARG             2
            106 UNPACK_EX              512
            108 STORE_FAST               4 (others)
            110 STORE_FAST               5 (second_last)
            112 STORE_FAST               6 (last)

 11         114 LOAD_FAST                4 (others)
            116 GET_ITER
        >>  118 FOR_ITER                10 (to 140)
            120 STORE_FAST               7 (item)

 12         122 LOAD_GLOBAL              0 (str)
            124 LOAD_FAST                7 (item)
            126 CALL_FUNCTION            1
            128 YIELD_VALUE
            130 POP_TOP

 13         132 LOAD_CONST               4 (', ')
            134 YIELD_VALUE
            136 POP_TOP
            138 JUMP_ABSOLUTE           59 (to 118)

 14     >>  140 LOAD_GLOBAL              0 (str)
            142 LOAD_FAST                5 (second_last)
            144 CALL_FUNCTION            1
            146 LOAD_CONST               5 (', and ')
            148 LOAD_GLOBAL              0 (str)
            150 LOAD_FAST                6 (last)
            152 CALL_FUNCTION            1
            154 BUILD_TUPLE              3
            156 GET_YIELD_FROM_ITER
            158 LOAD_CONST               0 (None)
            160 YIELD_FROM
            162 POP_TOP
            164 LOAD_CONST               0 (None)
            166 RETURN_VALUE

 10     >>  168 POP_TOP
            170 LOAD_CONST               0 (None)
            172 RETURN_VALUE

Answer 3

Naming: prefer specific over generic. The function name tells us nothing about its behavior or purpose. Better alternatives could mention the idea of joining, punctuation, or the Oxford comma.

Naming: prefer ordinary over awkward. The function's argument could be anything, so how do we name it? Not with something like l_list: it's needlessly awkward to type, visually off-putting, and tells us nothing more specific than several ordinary or conventional alternatives: items, vals, xs, etc.

Duck typing: Python generally embraces it. Whenever feasible, avoid imposing overly strict typing requirements on functions and methods. We don't actually care whether the function's argument is a list or even a sequence. Any iterable of values convertible to str will work. Many data types satisfy our needs: list[str], tuple[str], a dict with str keys, just a str (of characters), or endless user-defined objects.

Unnecessary quote marks. Your current code is adding single-quote marks to the returned string that I don't think are called for in the problem statement.

Simplifying the logic: fewer conditional branches. If there are 0, 1, or 2 values, you can just join them with ' and '. Otherwise, you do the Oxford comma routine -- which can be made more readable by popping off the last value.

Writing better demo code. It's great that you posted your question with a main() method to exercise your code. Even better would be to do the same thing covering the primary input scenarios: 0 values, 1, 2, 3, and 4+. An illustration is shown below. Better still would be a demo framed as a test with assertions: I was too lazy for that, but we have higher expectations for you!

def demo():
    vals = ['A', 'B', 'C', 99]
    for i in range(len(vals) + 1):
        xs = vals[0:i]
        text = oxford_comma_join(xs)
        print(xs, text)

def oxford_comma_join(xs):
    # Convert iterable to sequence of str.
    try:
        seq = [str(x) for x in xs]
    except Exception:
        raise ValueError

    # Return joined str.
    if len(seq) < 3:
        return ' and '.join(seq)
    else:
        last = seq.pop()
        return ', '.join(seq) + ', and ' + last

if __name__ == '__main__':
    demo()