Sending an SMS message using zenoss and python

Question

I have just finished a simple python script which uses this module to call Zenoss API for getting live events. What the script does:

• connects to zenoss
• reads a file which has one phone number on each line
• sends a message to each number by POSTing some data(first of all it authenticates) to a specific URL with the following parameters: phonenr, smstxt, Submit

The script speaks for itself and it doesn't need comments. What I'm looking for is some logic in the script / cases that I didn't covered / coding standards that are wrong / design patterns and maybe some advices on how I can improve the code ( in a way that will make each number receive the message at the same time - maybe using multithreading will help )

The file that contains the phone numbers looks like this:

phone_no1 phone_no2 .. phone_non 

The code is:

#!/usr/bin/env python # -*- coding: UTF-8 -*- import urllib2 as urllib from urllib import urlencode from os.path import join as joinPath from traceback import print_tb from os.path import isfile from sys import exc_info import myzenoss zenoss = myzenoss.Zenoss('ip_of_zenoss', 'admin', 'pass') hdrs = {'User-Agent': "Mozilla/5.0 (X11; U; Linux i686) Gecko/20071127 Firefox/2.0.0.11"} gh_url = 'http://ip-for-post/txsms.cgi' # basically this is an IP for a Mobilink which sends the SMS APPLICATION_PATH = '/srv/sms_alert/' ALERT_POINT_PATH = joinPath(APPLICATION_PATH, 'alert_contact') try: isInProduction = False for evt in zenoss.get_events(): if evt['prodState'] == 'Production': isInProduction = True if isInProduction and isfile(ALERT_POINT_PATH): alertContactContent = None with open(ALERT_POINT_PATH, 'r') as alertContactFile: alertContactContent = alertContactFile.read() alertContactContent = alertContactContent.splitlines() if alertContactContent: evt['summary'] = ':zenoss]\n\nError: {0}'.format(evt['summary']) evt['device']['text'] = '{0}'.format(evt['device']['text']) final_message = '{0}\nName: {1}\n\n'.format(evt['summary'], evt['device']['text']) for alertContactContentLine in alertContactContent: data = {'phonenr': alertContactContentLine, 'smstxt': str(final_message), 'Submit': 'SendSms'} data = urlencode(data) req = urllib.Request(gh_url, data, headers=hdrs) password_manager = urllib.HTTPPasswordMgrWithDefaultRealm() password_manager.add_password(None, gh_url, 'admin', 'password') auth_manager = urllib.HTTPBasicAuthHandler(password_manager) opener = urllib.build_opener(auth_manager) urllib.install_opener(opener) handler = urllib.urlopen(req) break # used this to only get the last event else: evt['summary'] = '#[ERROR: SMS ALERT NO CONTACT]# {0}'.format(evt['summary']) except Exception as e: ex_type, ex, tb = exc_info() print('\n #[ERROR]# TRANSFORM: exception: {ex}\n'.format(ex=e)) print('\n #[ERROR]# TRANSFORM: exception traceback: {trace}\n'.format(trace=print_tb(tb))) 

Answer 1

I would recommend using 'guard clauses'. Currently you have a large amount of indents that could be removed with these.
Take:

if isInProduction and isfile(ALERT_POINT_PATH): alertContactContent = None with open(ALERT_POINT_PATH, 'r') as alertContactFile: alertContactContent = alertContactFile.read() alertContactContent = alertContactContent.splitlines() 

This could be:

if not (isInProduction and isfile(ALERT_POINT_PATH)): continue alertContactContent = None with open(ALERT_POINT_PATH, 'r') as alertContactFile: alertContactContent = alertContactFile.read() alertContactContent = alertContactContent.splitlines() 

The latter is much easier to read. And even easier to read with multiple of these statements.

---

I'd also, like urllib2, recommend the Requests package.

See also: The Requests package is recommended for a higher-level http client interface.

---

I'd recommend not checking if ALERT_POINT_PATH is a file and just try and open it. This will prevent a TOCTOU bug.

To do this you can change:

if isInProduction and isfile(ALERT_POINT_PATH): alertContactContent = None with open(ALERT_POINT_PATH, 'r') as alertContactFile: alertContactContent = alertContactFile.read() alertContactContent = alertContactContent.splitlines() ... else: evt['summary'] = '#[ERROR: SMS ALERT NO CONTACT]# {0}'.format(evt['summary']) 

Into:

try: with open(ALERT_POINT_PATH, 'r') as f: alert_contact_content = f.readlines() except IOError: evt['summary'] = '#[ERROR: SMS ALERT NO CONTACT]# {0}'.format(evt['summary']) continue 

This significantly reduces the complexity of the algorithm.

This uses a correct tryexcept where you specify the error. And instead uses f.readlines() rather then alertContactFile.read().splitlines().

---

I'd recommend joining your formats into one line:

final_message = ':zenoss]\n\nError: {0}\nName: {1}\n\n'.format(evt['summary'], evt['device']['text']) 

And using a comprehension to pre-filter all the non evt['prodState'] == 'Production' evts.

---

In short I'd recommend:

def send_error_messages(zenoss): for evt in (e for e in zenoss.get_events() if e['prodState'] == 'Production'): try: with open(ALERT_POINT_PATH, 'r') as f: alert_contact_content = f.readlines() except IOError: evt['summary'] = '#[ERROR: SMS ALERT NO CONTACT]# {0}'.format(evt['summary']) continue if not alert_contact_content: continue final_message = ':zenoss]\n\nError: {0}\nName: {1}\n\n'.format(evt['summary'], evt['device']['text']) for phone_address in alert_contact_content: send_message({'phonenr': phone_address, 'smstxt': final_message, 'Submit': 'SendSms'}) break 

Where send_message is your urllib2 functions.

Answer 2

• The one coding standard is PEP8 and indeed it would be good if at least names are in common Python spelling, not like Java. E.g. it should be join_path (if you really want to rename it), not joinPath, etc.
• The names vs. positional arguments for format are a bit inconsistent; I'd say that unless you have a lot of arguments you can usually get by with unnamed positional instead, i.e. "{} {}".format(...).
• I don't like the tendency for stripping out characters in names unnecessarily: hdrs and evt aren't exactly much saving compared to headers and event.
• The import for os.path is mentioned twice, that could also be just one line instead.
• '{0}'.format(...) should probably be str(...) if you need the string conversion.

Now for the structure I see a couple of things.

• The try/catch block looks like it's not very much improving on what the Python interpreter would print by default.
• "Declaring" the isInProduction variable is not needed and considering it's set to True immediately it looks like it's not needed at all. Or the indentation is wrong, in which case it should instead be immediately initialised like is_in_production = event['prodState'] == 'Production'.
• Actually, initialising variables this way like again with alertContactContent is just not useful in Python. I'd likely move reading the file and splitting the content into a function and use that instead (like alert_contact_content = read_and_split(ALERT_POINT_PATH) or so).
• Again, the body of the innermost for-loop looks like it should be a function instead, like create_auth_manager() and should only be created once - unless I'm missing something all of those password_manager instances are the same, so it makes sense to reuse them instead.
• I don't understand the break. Or the comment on it. At all.

Basically reading all of this nested stuff would be much easier if it was structured, e.g. using functions or so.