HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Package for testing network connection

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
packagetestingfornetworkconnection

Problem

I wrote this package for monitoring the state of a network connection in the terminal because my net connection would go down for a few minutes at a time and come back up. I think it could be used in other applications.

I'm looking for any advice on how to improve or extend this package in any way possible.

Package repo

My concerns are as follows:

  • Is the package "Pythonic"?



  • Does the package contain any obvious bugs/errors/flaws?



```
#!/usr/bin/env python

'''
This module is for checking a networks status.
'''

import socket
from time import sleep as _sleep
from functools import wraps
import errno
import os
import signal

TIMEOUT = 2

class NetworkStatus(object):
def timeout(seconds=1, error_message=os.strerror(errno.ETIME)):
def decorator(func):
def _handle_timeout(signum, frame):
return
signal.signal(signal.SIGALRM, _handle_timeout)
signal.alarm(seconds)
def wrapper(*args, **kwargs):
try:
result = func(*args, **kwargs)
finally:
signal.alarm(0)
return result

return wraps(func)(wrapper)

return decorator

def set_timeout(self, t_out):
'''
Update the global variable TIMEOUT.
'''
global TIMEOUT
TIMEOUT = t_out

def sleep(self,delay):
'''
Sleep function with cleanup.
'''
try:
_sleep(delay)
except KeyboardInterrupt:
exit()

@timeout(TIMEOUT)
def check(self, **kw):
'''
Check if the network is up.
'''
self.sleep(TIMEOUT)
try:
socket.gethostbyname(kw.get('remote_server', 'www.google.com'))
return True
except Exception:
return False

def online(self, **kw):
'''
return True is the network is UP.
'''
return self.check(**kw) is True

def o

Solution

Importing sleep as _sleep is an awkward work around. It'd be much easier to do a plain import time, then in your sleep function refer to time.sleep. No need for odd aliases and nothing gets overwritten.

This function does nothing:

def _handle_timeout(signum, frame):
    return
    signal.signal(signal.SIGALRM, _handle_timeout)
    signal.alarm(seconds)


It will immediately return, and the following two lines will never be executed.

If TIMEOUT can be set then it's not a constant, and it should instead be a class level variable. Place it directly after the class definition, then you can directly assign it easily without needing to resort to the global keyword. You also don't need to pass it as an argument to @timeout, you can just use NetworkStatus.timeout directly there.

class NetworkStatus(object):

    timeout = 2


In online you're returning if self.online(kw) is True, but this is the same as returning self.online(kw) already is True. And check only returns true or false. There's no reason to have two separate functions here at all, just rename the vague check function to is_online and you're returning a boolean directly from there. Then you also have offline which makes little sense. If you're concerned about readability, then if not network.is_online() is plenty readable without having a specific function.

Further down you also have if_online and if_offline... what are these wrappers for? They seem overly generic to achieve one small goal (only run a function if the network is online/offline. It'd make more sense to have these specific to the function, rather than passing the function as an argument. Did you have a lot of functions that needed this construct? It doesn't look like you've used it here.

I'd rename mainloop to just run, or run_test. mainloop is a generic name that tells us nothing. Also, no need to store the result in up. You can evaluate directly:

if self.is_online():


Now that I'm here I notice that you never passing any keyword arguments as kw**. Indeed you never use them, instead just using string literals:

socket.gethostbyname(kw.get('remote_server', 'www.google.com'))


This is very bad practice! Those values should be constants or class attributes. You should also remove all the unnecessary kw** parameters. Why let the user pass in values that'll never be used for anything?

Code Snippets

def _handle_timeout(signum, frame):
    return
    signal.signal(signal.SIGALRM, _handle_timeout)
    signal.alarm(seconds)
class NetworkStatus(object):

    timeout = 2
if self.is_online():
socket.gethostbyname(kw.get('remote_server', 'www.google.com'))

Context

StackExchange Code Review Q#110263, answer score: 6

Revisions (0)

No revisions yet.