9
\$\begingroup\$

For an external application I need to send a command as a string, like this:

["START", "1", "2", "3", "4", "STOP"]

Note the double quotes!

I create this command with the following function:

def create_command(amount):
    command = ["START"]
    list = create_list(amount)
    command += list
    command += ["STOP"]
    command = str(command ) 
    command = command.replace("\'", "\"")
    return command

And I create a list from a given number with the following function:

def create_list(data):
    list = []
    data = str(data)
    for letter in data:
        list.append(letter)
    return list

Is the a way to make both functions more pythonic and not so straightforward? I don't really like them now, they look a bit clumpsy and I think there must be a better way do the thing.

\$\endgroup\$
10
\$\begingroup\$

create_list is building a list of all the items in the string form of data. And so you can change it to:

def create_list(data):
    return list(str(data))

I find it easier to read create_command if you merge some of the lines together:

def create_command(amount):
    command = ["START"] + create_list(amount) + ["STOP"]
    return str(command).replace("\'", "\"")

And so you can merge the above two changes together:

def create_command(amount):
    command = ["START"] + list(str(amount)) + ["STOP"]
    return str(command).replace("\'", "\"")

Expanding further, rather than using str.replace you can use json.dumps to format the list. This has the benefit that it will escape ' and " characters for you, which your code doesn't correctly handle. (Thanks Mathias)

import json


def create_command(amount):
    command = ["START"] + list(str(amount)) + ["STOP"]
    return json.dumps(command)
\$\endgroup\$
  • 3
    \$\begingroup\$ You could also make composing the list a bit neater using iterable unpacking: ["START", *str(amount), "STOP"] (available since Python 3.5). \$\endgroup\$ – Arthur Tacca Jul 5 '18 at 16:33
  • \$\begingroup\$ Why on Earth is json buried at the bottom of your answer? \$\endgroup\$ – jpmc26 Jul 6 '18 at 0:51
  • \$\begingroup\$ @jpmc26 because my primary purpose is to review the code, and so I work my way up to the best code. It wouldn't make sense to go the other way. \$\endgroup\$ – Peilonrayz Jul 6 '18 at 1:32
  • \$\begingroup\$ It makes sense for the best code to be the first thing a reader sees so that they don't get confused about where you're headed. It also makes it harder to miss the best code when doing an initial skim. \$\endgroup\$ – jpmc26 Jul 6 '18 at 18:34
  • \$\begingroup\$ @jpmc26 what you're suggesting sounds like you want me to promote people to just steal my code and not learn anything. I want people to learn something, and so won't make you suggested changes. \$\endgroup\$ – Peilonrayz Jul 6 '18 at 18:56
10
\$\begingroup\$

Don't overshadow built in functions

list is already a built in. When you would try to use list() later on you'd get an Error.

Alternative

You need to send a string. Instead of creating a list and then converting to a string you can build the string from the start.

def create_list(data):
    return ', '.join(['"{}"'.format(c) for c in str(data)])

def create_cmd(data):
    return '["START", {}, "STOP"]'.format(create_list(data))
\$\endgroup\$
  • \$\begingroup\$ Oh, thanks! I don't actually use list as a name, I just changed the variable names for the question and absolutely forgot that there is a function with this name. \$\endgroup\$ – Alex.S Jul 5 '18 at 12:37
  • 1
    \$\begingroup\$ Eh. list is an obvious name, and shadowing names in a small scope isn’t at all problematic. I agree with the rest of your answer. \$\endgroup\$ – Konrad Rudolph Jul 5 '18 at 16:11
  • \$\begingroup\$ @KonradRudolph Sure in this small scope it doesn't really matter, but it is not a good habit to get into. And is worth mentioning at a code review \$\endgroup\$ – Ludisposed Jul 5 '18 at 16:22
4
\$\begingroup\$

The python json.dumps function from stdlib can do this for you.

from json import dumps
result = build_your_list()  # ['a', 'b']
dumps(result)  # '["a", "b"]'

EDIT: I only just noticed that using this was recommended at the bottom of an already existing answer - but I think this deserves a full answer. I consider it much cleaner than the potentially inaccurate string manipulation answers.

\$\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.