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.

I use at to schedule jobs and atq to display the scheduled jobs. But it mildly irritates me in that I have to look up each job individually. I wrote a Python script to display the scheduled job under the scheduled time.

(In my world all the commands will begin 'task')

So instead of

stafflinux$atq
8   Mon Aug 18 09:00:00 2014 a joseph
10  Tue Aug 19 09:00:00 2014 a joseph
15  Thu Aug 21 09:00:00 2014 a joseph
12  Fri Aug 22 09:00:00 2014 a joseph
9   Thu Aug 21 09:00:00 2014 a joseph
14  Sat Aug 30 09:00:00 2014 a joseph
7   Sun Aug 17 09:00:00 2014 a joseph
6   Mon Aug 18 09:00:00 2014 a joseph
11  Sat Aug 30 09:00:00 2014 a joseph
stafflinux$

My script produces

stafflinux$./atscript.py 
8  Mon Aug 18 09:00:00 2014 a joseph
task "buy a four-way plug adapter" 
task "see guy about meeting" 
-----------------------------------------
10  Tue Aug 19 09:00:00 2014 a joseph
task "bring back personal effects from office" 
-----------------------------------------
15  Thu Aug 21 09:00:00 2014 a joseph
task "book tickets for next week"
-----------------------------------------

I'm looking for any feedback - particularly in terms of 'pythonic' style and any and all tricks I may have missed:

#!/usr/bin/python
import os
os.system("atq > attemp.txt") 
file = open("attemp.txt")
line = file.readline()
while line: 
    number =line[:2]
    print line.strip()
    os.system("at -c "+ number+ "| grep task")
    line=file.readline()
    print '-----------------------------------------'
print line
os.system("rm attemp.txt")
share|improve this question

2 Answers 2

Your script creates a temporary file unnecessarily, uses os.system which is not recommended, and other messy things. Consider this alternative:

#!/usr/bin/env python

import subprocess
import re

re_job_number = re.compile(r'\d+')

for atq_line in subprocess.Popen(['atq'], stdout=subprocess.PIPE).stdout:
    job_number = re_job_number.search(atq_line).group()
    print atq_line.strip()
    for at_line in subprocess.Popen(['at', '-c', job_number], stdout=subprocess.PIPE).stdout:
        if at_line.startswith('task '):
            print at_line.strip()
    print '-' * 40

Advantages:

  • No need for a temporary file to save the atq output, you can process it directly
  • For running shell commands, subprocess is the recommended way, os.system is discouraged
  • No need for a grep process, you can handle the output filtering of at -c in Python itself

Also, writing the first line this way makes the script more portable:

#!/usr/bin/env python

If you write as #!/usr/bin/python then you are effectively hard-coding the Python path, and it might not be the same in all systems.

Finally, I recommend writing print(something) instead of print something, to keep your script ready for migrating to Python 3 someday.

share|improve this answer
1  
Almost exactly what I was thinking of, except I would have written job_number = re.search('\d+', atq_line).group(0). –  200_success Aug 15 at 18:19
1  
Hard-coding #!/usr/bin/python is not necessarily a bad idea — it's a judgement call. –  200_success Aug 15 at 18:20
    
Good point about re.search, and also the variable naming. Updated my post, thanks! –  janos Aug 15 at 18:30

You should segment your code in a way that makes it more readable.

#!/usr/bin/python
import os

os.system("atq > attemp.txt") 
file = open("attemp.txt")
line = file.readline()

while line: 
    number =line[:2]
    print line.strip()
    os.system("at -c "+ number+ "| grep task")
    line=file.readline()
    print '-----------------------------------------'

print line
os.system("rm attemp.txt")

Additionally, you should use the with context manager, which takes care of file closing and error handling for you. Also, you should avoid using strings all over the place and store the filename in a variable. You should also avoid setting unnecessary variables (like number). Additionally, use Python's string multiplication abilities ('-' * 40 will print 40 '-'). Your script would now look like:

#!/usr/bin/python
import os

filename = 'attemp.txt'
os.system("atq > " + filename)

with open(filename, 'r') as handle:
    for line in handle:
        print line.strip()
        os.system('at -c ' + line[:2] + ' | grep task')
        print ('-' * 40) + "\n" + line

os.system("rm " + filename)

This is untested code, but it should work the same as your original code.

share|improve this answer
    
Do note that it will remove your attemp.txt, which I'm not sure if its truly intended to do so (I'd honestly keep the txt file just in case I had to re-run it) –  jsanc623 Aug 15 at 17:46

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.