Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I need some feedback here. Due to a weird issue on my server system, the network card disconnects from the network regularly. I'll fix this at a later date. But I wanted to know if the below script can be improved.

It's comprised of two files. A python script (autoConnect.py), and a Bash script (checkOnline.sh).

autoConnect.py:

#!/usr/bin/python
from subprocess import call, Popen, PIPE, STDOUT
import time
cmd = './checkOnline.sh'
while True:
    p = Popen(cmd, shell=True, stdin=PIPE, stdout=PIPE, stderr=STDOUT, close_fds=True)
    if "0" in p.stdout.read():
        print time.ctime() + ": Offline"
        print "Attempting to reconnect..."
        print "Determining network profile..."
        cmdTwo = "wicd-cli -ySl | sed -n '2 p' | grep -i paws -c"
        pTwo = Popen(cmdTwo, shell=True, stdin=PIPE, stdout=PIPE, stderr=STDOUT, close_fds=True)
        if "1" in pTwo.stdout.read():
            print "Network profile is \"1\""
            defNum = 1
        else:
            print "Network profile is \"2\""
            defNum = 2
        print "Connecting to network..."
        p = Popen(cmd, shell=True, stdin=PIPE, stdout=PIPE, stderr=STDOUT, close_fds=True)
        while "0" in p.stdout.read():
            call("wicd-cli -yn " + defNum + " -c")
            p = Popen(cmd, shell=True, stdin=PIPE, stdout=PIPE, stderr=STDOUT, close_fds=True)
            time.sleep(3)
            if "0" in p.stdout.read():
                print "Failed to connect. Trying again..."
        print "Success, connected to network!"
    else:
        print time.ctime() + ": Online"
    time.sleep(5)

checkOnline.sh:

#!/bin/bash

host=google.com

(ping -w5 -c3 $host  > /dev/null 2>&1) && echo "1" || (echo "0" && exit 1)

The former is the main file to be run. checkOnline pings google.com 3 times for 5 seconds, if nothing is returned within 5 seconds, it will return 0. If it gets something, it returns 1 into stdout.

I wanted to know if this can be improved for what I want it to do (check if the network is online, search for networks, connect, repeat). As of right now, I'm away from my server's physical location and am unable to test it on the current system I'm using, but the script should be functional.

share|improve this question
Welcome to Code Review, any code you want reviewed needs to be posted in the question itself. – Winston Ewert♦ Oct 29 '11 at 16:33
Right. Added that. – dragos240 Oct 29 '11 at 16:59
1  
Good. The only thing for future reference is that the plain english description of your algorithm isn't really necessary. We are all coders here, we read code. The english is only helpful if your algorithm is incomphrensible, which isn't the case here. – Winston Ewert♦ Oct 29 '11 at 17:30
Right. Okay, I'll remove that. – dragos240 Oct 29 '11 at 17:32

1 Answer

up vote 7 down vote
  1. I'd move the contents of the script into a main function and then use the if __name__ == '__main__' boilerplate to start it.
  2. I wouldn't use external tools like bash/sed/grep to do additional logic. I'd implement that logic in python.
  3. I'd use the communicate method for POpen objects rather then reading the stdout attribute
  4. Your POpens are all very similiar. I'd write a function that takes a command line executes it and returns the standard output
  5. You check if you are connected in multiple places, write a am_connected() function which returns True or False and use it multiple times
  6. I'd probably write a function for each external command you execute even if they are not repeated because it'll make your code cleaner.
share|improve this answer
Thanks! I'll get right on that. – dragos240 Oct 29 '11 at 18:14

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.