6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review, any code you want reviewed needs to be posted in the question itself. \$\endgroup\$ – Winston Ewert Oct 29 '11 at 16:33
  • \$\begingroup\$ Right. Added that. \$\endgroup\$ – dragos240 Oct 29 '11 at 16:59
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Winston Ewert Oct 29 '11 at 17:30
  • \$\begingroup\$ Right. Okay, I'll remove that. \$\endgroup\$ – dragos240 Oct 29 '11 at 17:32
7
\$\begingroup\$
  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.
| improve this answer | |
\$\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.