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 know the code is full of useless data and stylistic and analysis errors and I'd really love and appreciate to have a pro's idea about it so I can learn to code (and think) better.

import os, time
from ConfigParser import SafeConfigParser
configfilename = "imhome.ini"
scriptpath = os.path.abspath(os.path.dirname(__file__))
configfile = os.path.join(scriptpath, configfilename)
parser = SafeConfigParser()
parser.read(configfile)
ip_smartphone = parser.get('imhome', 'ip_smartphone')
mac_address_pc = parser.get('imhome', 'mac_address_pc')
hometime_path = parser.get('imhome', 'hometime_path')
in_path = parser.get('imhome', 'in_path')
mp3_path = parser.get('imhome', 'mp3_path')
maximum_time_before_logout = parser.get('imhome', 'maximum_time_before_logout')
if os.path.exists(hometime_path) == False:
        os.system ("touch " + str(hometime_path))
if os.path.exists(in_path) == True:
        os.system ("rm " + str(in_path))
finished = 1
while finished == 1:
    check_presence = os.system ("ping -w 1 " + str (ip_smartphone) + " >/dev/null")
    file_modification_time = (os.path.getmtime(hometime_path))
    file_delta_time = time.time() - file_modification_time
    if check_presence == 0 :
            os.system ("rm " + str(hometime_path))
            change_arrived=("echo " + str(check_presence) + " > hometime")
            os.system (change_arrived)

            if os.path.exists(in_path) == False:
            # You can add any custom commands to execute when home below this row

                os.system ("etherwake " + str(mac_address_pc))            
                os.system ("mpg123 " + str(mp3_path) + " >/dev/null")
                os.system ("touch " + str(in_path))
            # stop adding commands

    elif check_presence == 256 and file_delta_time > int(maximum_time_before_logout):
        if os.path.exists(in_path) == True:
                    os.system ("rm " + str(in_path))
share|improve this question

3 Answers 3

up vote 11 down vote accepted

1. Comments on your code

  1. You make quite a lot of use here of os.system, which runs a command in a subshell. This can be a risky function to use, because the shell can do anything. For example, you write

    os.system ("rm " + str(hometime_path))
    

    but what if hometime_path were -rf *?

    You're probably safe in this particular program because it's only for your own use and because the strings come from your own configuration file. But it's worth getting into the habit of doing this kind of thing properly, so that in situations which really are risky, you'll know what to do.

    In the case of rm you can use os.unlink:

    os.unlink(hometime_path)
    

    But in the general case, use subprocess.call and use the -- argument (if necessary) to make sure that the arguments you pass cannot be misinterpreted as options. For example,

    subprocess.call(['rm', '--', hometime_path])
    

    In a case where you want to redirect standard output, like this:

    os.system ("ping -w 1 " + str (ip_smartphone) + " >/dev/null")
    

    you can use the stdout keyword argument to subprocess.call:

    subprocess.call(['ping', '-w', '1', '--', ip_smartphone],
                    stdout = open('/dev/null', 'w'))
    
  2. Most of your calls to str are unnecessary: these configuration parameters ought to be strings already. (If they are not, I think it you'd prefer to get an exception so that you could fix the problem, rather than silently coercing the incorrect value to a string.)

  3. You don't need to compare booleans explicitly to True and False. So instead of:

    if os.path.exists(in_path) == True:
    

    just write:

    if os.path.exists(in_path):
    

    and instead of:

    if os.path.exists(hometime_path) == False:
    

    just write:

    if not os.path.exists(hometime_path):
    
  4. In this code you remove a file only if it exists:

    if os.path.exists(in_path) == True:
        os.system ("rm " + str(in_path))
    

    But the rm command already has a option for this, so you could write instead:

    subprocess.call(['rm', '-f', '--', in_path])
    

    Or do it in pure Python:

    try:
        os.unlink(in_path)
    except OSError: # No such file or directory
        pass
    

    (This is an example of how it is often better to ask for forgiveness than permission.)

  5. Assuming that " > hometime" is a mistake for " > " + hometime_path (as suggested in another answer), then in this bit of code:

    os.system ("rm " + str(hometime_path))
    change_arrived=("echo " + str(check_presence) + " > hometime")
    os.system (change_arrived)
    

    you remove a file and then overwrite it with the value of check_presence. There's no need to remove the file first: the second command will overwrite it if it exists.

    And in any case this is easy to do in pure Python:

    with open(hometime_path, 'w') as f:
        f.write('{}\n'.format(check_presence))
    
  6. You use the modification time of the file hometime_path to remember the last time that your ping command succeeded. Why not remember this time in a variable in Python and avoid having to create and delete and examine this file?

  7. You use the existence of the file in_path to remember whether your smartphone is connected. Why not remember this fact in a variable in Python and avoid having to create and delete and examine this file?

2. Rewrite

Making the above improvements results in something like this (but this is untested, so beware):

import os, subprocess, time
from ConfigParser import SafeConfigParser
from datetime import datetime, timedelta

configfilename = "imhome.ini"
scriptpath = os.path.abspath(os.path.dirname(__file__))
configfile = os.path.join(scriptpath, configfilename)
parser = SafeConfigParser()
parser.read(configfile)
ip_smartphone = parser.get('imhome', 'ip_smartphone')
mac_address_pc = parser.get('imhome', 'mac_address_pc')
mp3_path = parser.get('imhome', 'mp3_path')
timeout = timedelta(seconds = int(parser.get('imhome', 'maximum_time_before_logout')))

devnull = open('/dev/null', 'wb')
connected = False
disconnect_time = None

while True:
    ping = subprocess.call(['ping', '-w', '1', '--', ip_smartphone],
                           stdout = devnull)
    if ping == 0:
        disconnect_time = datetime.now() + timeout
        if not connected:
            connected = True
            subprocess.call(['etherwake', mac_address_pc])
            subprocess.call(['mpg123', mp3_path], stdout = devnull)
    elif ping == 256 and connected and datetime.now() > disconnect_time:
        connected = False
    time.sleep(1)
share|improve this answer
    
This is seriously great fuel for my brain! I am VERY thankful also for the time you took to explain and incredibly rewrite the code! I'll hide it in my drawer and try to write it on my own to check how I do compared to you. If you happen to be in central Italy you have a dinner paid! :D –  Pitto Jan 18 '13 at 13:41
    
I'm finally back from my holidays and I am trying to understand better your work... I can't get how you handle time... If I print the variables to understand what is going on I still can't get it. The main problem is timeout. in timeout you get the number of seconds defined in the config file, right? And then you use this to calculate disconnect_time adding this very moment to maximum time before logout? Why? Maybe it's just a mistake (you told me it wasn't checked) but I am a newbie so... :) –  Pitto Jan 28 '13 at 13:56
    
The idea is that disconnect_time is the clock time at which the program will consider the smartphone to be disconnected if it hasn't received any more ping responses by then. Each time a ping response is received, we update diconnect_time to a be a bit further in the future. But if we don't receive any ping responses, we leave it unchanged, until eventually datetime.now() > disconnect_time. (This is a standard idiom for timeout processing.) –  Gareth Rees Jan 28 '13 at 14:03
    
Mmmh ok... But why to you sum it with timeout (which is the value included in my config file)? –  Pitto Jan 28 '13 at 14:29
    
Suppose that your config file has timeout = 15 seconds. Then each time we receive a ping response, we set disconnect_time = datetime.now() + timeout. That is, disconnect_time now represents a time 15 seconds in the future. If we haven't received any more ping responses by then, we'll consider the smartphone to be disconnected. –  Gareth Rees Jan 28 '13 at 14:33

Neat idea with that script. It looks good; not bad for the first program. Just a few comments:

  1. I would improve some variable names: file_modification_time -> phone_last_seen_time, file_delta_time -> phone_unseen_duration. When I was reading the program it took me some time to figure out their purpose.
  2. Use while True: instead of while finished == 1: so it is immediately clear that it is an infinite loop.
  3. In the line change_arrived=("echo " + str(check_presence) + " > hometime") you probably want str(hometime_path) instead of "hometime".
  4. It would be also good to describe purpose of some variables in comments. Mainly hometime_path and in_path.
share|improve this answer

I would add a time.sleep(1) call at the end of the loop. Without that I had to kill the program from the shell to stop it, as it was not responding to Ctrl-C.

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.