Python siren control

Question

Below is some code I've put together to contol a siren for a fire service.

It works by webscraping a paging feed and looks for set triggers.

Is there a better way of doing my code or is this "pythonic" enough or can it be improved?

Im python self taught.

"""PYTHON SCRIPT TO CONTROL SIREN 
This script will webscape a paging feed and
in turn activate a siren on set incidents
siren should only sound for 30secconds between 0800 -2000 hrs
this has been made so the siren only goes off for some incidents not all
by s.rees (c) 2016
"""
from scraper import Scraper
import sys
import requests
from clint.textui import puts, colored
import RPi.GPIO as GPIO
from time import sleep
import time
time = datetime.datetime.now()
print "Starting"
print "Monitoring"
#Set GPIO siren is attached too
GPIO.setmode(GPIO.BOARD)
GPIO.setup(7, GPIO.OUT)
def siren ():
 GPIO.output(7, GPIO.HIGH)
 sleep(30)
 GPIO.output(7, GPIO.LOW)
if __name__ == "__main__":
# Load webscraper module
 scraper = Scraper(5, recent_messages=True)
 @scraper.register_handler
 def handler(msg):
# get msg from scraper and make it readable 
 puts(colored.yellow(msg.channel), newline=False)
 puts(" [", newline=False)
 puts(colored.green(msg.datetime), newline=False)
 puts("] ", newline=False)
 if msg.response:
 puts(colored.red(msg.text))
 else:
 puts(msg.text)
# set agency
 if 'MFS' in msg.channel:
#set unit
 if 'Station' in msg.channel:
 print "Brigade Found"
#set response type
 if 'RESPOND RUBBISH OR WASTE' in msg.text:
 print "Trigger found"
 if (time > datetime.time(8) and time < datetime.time(20)):
 print "Activate Siren"
 siren ()
 print 'done'
 return
 else:
 print "out of time restrictions"
 return
 if 'RESPOND GRASS FIRE' in msg.text:
 print "Trigger found"
 if (time > datetime.time(8) and time < datetime.time(20)):
 print "Activate Siren"
 siren ()
 print 'done'
 return
 else:
 print "out of time restrictions"
 return
 else:
 print "Brigade found but no trigger"
 return
 else:
 return
 scraper.run()
 GPIO.cleanup()

• Vogel612: Hey there, I've rolled back the edit you made to this post. For one, just editing the post does not remove the content from the website. Secondly With accepting the Terms of Service and posting here, you licensed your work under CC-BY-SA. If you need to remove this for legal reasons you can try contacting StackOverflow Inc. through the [contact] option. Finally the lIcense gives you the right to dissociate your post from your name. Poke me in Code Review Chat or raise a custom flag if you have questions :) Thanks!

Answer 1

Here are some code style and organization notes:

• remove unused imports (sys, requests)
• import time is also not needed since you are immediately shadowing it after by defining the variable time
• missing datetime import
• organize imports per PEP8
• use consistent 4-space indents
• no need for extra space after the function and before the opening parenthesis - e.g. siren() vs siren ()
• let's avoid hardcoding the constant values, like the number 7 and define it in a, say, GPIO_PIN "constant"
• let's move all the initial setup code to under the "main" block of the program
• use print() as a function as opposed to a statement (for Python 3.x compatibility)

Here is the improved initial part of the code without the "scraping" part:

from datetime import datetime
from time import sleep
import RPi.GPIO as GPIO
from clint.textui import puts, colored
from scraper import Scraper
GPIO_PIN = 7
def setup_gpio():
 """Sets GPIO siren is attached too."""
 GPIO.setmode(GPIO.BOARD)
 GPIO.setup(GPIO_PIN, GPIO.OUT)
def siren():
 """Make a siren (flash) on a desired output pin."""
 GPIO.output(GPIO_PIN, GPIO.HIGH)
 sleep(30)
 GPIO.output(GPIO_PIN, GPIO.LOW)
if __name__ == "__main__":
 print("Starting")
 print("Monitoring")
 now = datetime.now()
 setup_gpio()

Note that I've renamed time variable to a more readable now.

---

As far as the rest of the code in the "main" block of the program:

• please see if there is a better way to register a handler for the scraper - instead of defining a decorator, define the handler function before the "main" block and call register_handler() passing in the function:

  scraper.register_handler(handler)
  

• the time comparisons can be improved by using the .hour attribute of a datetime and short-circuiting:

  if 8 < now.hour < 20:
  

• you can omit the last "else" + "return" branch

• the prints around the siren() call should probably be put into the siren() function itself to avoid the code duplication

• shaggs: Thanks look better.... could i handle the if statements any better?
• alecxe: @shaggs added some notes about the rest of the code. If I would have been familiar with the scraper library or the clint tool, may be I could've added some more points about that too. But, refactoring is a never-ending incremental process - you may even apply these suggestions and put the code to the code review again for a second round :)
• shaggs: Would it be better to make the GPIO and TRIGGERS read from a config file?
• alecxe: @shaggs or, you can make them command-line arguments with some default values..