7
\$\begingroup\$

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()
\$\endgroup\$
1
  • 1
    \$\begingroup\$ 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! \$\endgroup\$ – Vogel612 Sep 26 '18 at 12:27
5
\$\begingroup\$

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
\$\endgroup\$
4
  • \$\begingroup\$ Thanks look better.... could i handle the if statements any better? \$\endgroup\$ – shaggs Mar 9 '17 at 18:28
  • \$\begingroup\$ @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 :) \$\endgroup\$ – alecxe Mar 9 '17 at 19:11
  • \$\begingroup\$ Would it be better to make the GPIO and TRIGGERS read from a config file? \$\endgroup\$ – shaggs Mar 10 '17 at 0:32
  • \$\begingroup\$ @shaggs or, you can make them command-line arguments with some default values.. \$\endgroup\$ – alecxe Mar 10 '17 at 1:16

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.