current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I'm a Python noob. This is the code I'm using:

for i in argv[2:]:                                 # argv[1] isn't required by s.py
    if ' ' in i:
        argv[argv.index(i)] = '"%s"' % i
args = ' '.join(argv[2:])
os.system('s.py '+args)

Is there a more elegant way to do this?

share|improve this question

2 Answers 2

up vote 7 down vote accepted

You shouldn't use os.system to run commands. You should use subprocess instead, for example:

from sys import argv
import subprocess

cmd = 's.py'
args = [cmd] + argv[2:]
subprocess.call(args)

This is a lot cleaner. In your version you added quotes around the command line arguments that contained spaces. (Which btw would break spectacularly if the command line arg also contained quote character...) Using subprocess, you don't need such dirty hacks.

share|improve this answer
    
I also have to pass python along with s.py or it says [Error 193] %1 is not a valid Win32 application –  Zuck 2 days ago
1  
Instead of passing along python, better go with sys.executable. It is always a good idea to have as few constant values as possible in your script, and sys.executable always points to the python executable. –  Thor yesterday
up vote 4 down vote

Janos makes a good point about the code overall, but there are a few specific points on Python looping you should be aware of.

Firstly, you can map a function to all elements in a list:

args = " ".join(map(lambda s: '"%s"' % s if ' ' in s else s, args[2:]))

Secondly, naming an element, rather than its index, i is likely to confuse readers of your code.

Finally, list.index gives you the first index, which may not play well with duplicates. If you need both an element and its index you should use enumerate:

for i, s in enumerate(args[2:], 2):
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.