Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I use this to automate the lights in my home while I am away at night (simulating presence). The scheduling of when this runs is handled by my home automation software (Indigo Domotics).

I'm open to any/all critique, but am especially interested in whether doing the randomization within the Light object is the right approach (and if the method of randomizing is good/pythonic/efficient). I'm a self taught programmer as a hobby only and am always trying to refine my thinking around code structure.

#!/usr/bin/python

from datetime import datetime
import random


# (min/max) seconds to delay before light turns on (randomized every time)
start_delay_range = (15, 70)
# (min/max) brightness (0-100, randomized every time)
brightness_range = (50, 85)
# (min/max) duration for light to stay on (randomized every time)
duration_range = (300, 900)


class Light():
    '''
    Define a light object.

    Attributes:
        name (str): Name of the light.
        device_id (int): The Indigo ID of the device.
        dimmable (bool): Set the light as dimmable or not.
    '''
    def __init__(self, name, device_id, dimmable):
        self.name = name
        self.device_id = device_id
        self.dimmable = dimmable

    def brightness(self):
        '''Return random brightness within <brightness_range>.'''
        return random.randint(brightness_range[0], brightness_range[1])

    def duration(self):
        '''Return random duration within <duration_range>.'''
        return random.randint(duration_range[0], duration_range[1])

    def start_delay(self):
        '''Return random delay within <start_delay_range>.'''
        return random.randint(start_delay_range[0], start_delay_range[1])

    def run(self):
        '''Turn on the light to <brightness> after <start_delay> for <duration>.'''
        if self.dimmable:
            # Turn on light
            indigo.dimmer.setBrightness(self.device_id,
                                        value = self.brightness(),
                                        delay = self.start_delay())
        else:
            # Turn on light
            indigo.device.turnOn(self.device_id,
                                 self.start_delay())

        # Turn off light after <delay>
        indigo.device.turnOff(self.device_id,
                              delay = self.duration())

# List of Light() objects to randomize
lights = [
    Light(name='Breakfast Room', device_id=1222428814, dimmable=True),
    Light(name='Kitchen Cabinet', device_id=18462733, dimmable=False),
    Light(name='Hallway', device_id=93680547, dimmable=True),
    Light(name='Living Room Recessed', device_id=7507220, dimmable=True),
    Light(name='Stairs', device_id=1842915774, dimmable=True),
    Light(name='TV Room', device_id=1569858222, dimmable=True)
]


def main():
    for light in lights:
        light.run()

if __name__ == '__main__':
    main()
share|improve this question
    
Welcome to Code Review! When you turn the device off, shouldn't the startDelay be added to the duration? (or rename duration to stopDelay) – oliverpool Mar 1 at 6:59
    
I see what you mean, it might be more consistent to call it stop_delay. Duration is more accurate maybe, I'm torn on this one. I'll think about it! – Spencer Mar 2 at 0:42
up vote 1 down vote accepted

This code looks good! But I still have some notes to clean it up a bit.

First, all your constants (start_delay_range, brightness_range, and duration_range) should really be part of the Light class since they're used specifically in there. If that's where they're relevant, keep them there. Plus this makes it easier to do from light_tools import Light and not be caught without necessary values.

class Light():
    '''
    Define a light object.

    Attributes:
        name (str): Name of the light.
        device_id (int): The Indigo ID of the device.
        dimmable (bool): Set the light as dimmable or not.
    '''

    # (min/max) seconds to delay before light turns on (randomized every time)
    START_DELAY_RANGE = (15, 70)
    # (min/max) brightness (0-100, randomized every time)
    BRIGHTNESS_RANGE = (50, 85)
    # (min/max) duration for light to stay on (randomized every time)
    DURATION_RANGE = (300, 900)

Next, you might want to use the @property decorator for your functions. Decorators are really interesting to look into, but in this case @property is a simple builtin that means you can use the function like an attribute of your class that's evaluated each time. Simply put, you don't need to call it any more.

@property
def start_delay(self):
    '''Return random delay within <start_delay_range>.'''
    return random.randint(start_delay_range[0], start_delay_range[1])

def run(self):
    '''Turn on the light to <brightness> after <start_delay> for <duration>.'''
    if self.dimmable:
        # Turn on light
        indigo.dimmer.setBrightness(self.device_id,
                                    value = self.brightness,
                                    delay = self.start_delay)

Notice that it's now just self.brightness? It's still evaluating brightness each time you call it, but the syntax looks better. It's more like an attribute than a function call that could do who knows what (even though semantically you could still do anything in the function, you're signalling that this is a simple attribute being returned).

One minor point is that I would move the lights list into the if __name__ == '__main__' block. It will still be available in the global namespace, but it wont run any more unless the script is actually run. So again, if someone wants to import Light they wont get this strange recurring slowdown with every import.

My last point is debatable, but personally I would add an optional parameter to Light to immedidately start running.

def __init__(self, name, device_id, dimmable, start=False):
    self.name = name
    self.device_id = device_id
    self.dimmable = dimmable
    if start:
        self.run()

Now instead of having to always call light.run() after instantiation, you have the option to just pass start=True and have it run as soon as it's created. Generally I don't like having boilerplate set up at the start of a class. If it needs some code to properly be set up, that's a prime candidate for __init__.

share|improve this answer
    
Thanks so much for the detailed response! I've always seen @property in other peoples code but never really looked into it, thanks for the simple explanation. I'll be investigating them further for sure. Good call on the constants being inside the Light class. I'll definitely be incorporating these changes, cheers :) – Spencer Mar 2 at 0:39

Looking at the random number generation, a lot of duplicated code can be easily removed.

Starting at the 4th line: import random. The only function that is used is randint, so you could do from random import randint and then use randint everywhere, instead of random.randint.

Another repetition that can be avoided concern the arguments of randint: you can unpack the tuple using the *.

def start_delay(self):
    '''Return random delay within <start_delay_range>.'''
    return randint(*start_delay_range)

And your (repetitive) properties are now much shorter!

share|improve this answer
    
Good point on only importing randint, thanks. SUPER cool about unpacking, I never knew about this! – Spencer 2 days ago
1  
Unpacking in python is very nice and can be used with almost all kind of collections (list, tuple...) – oliverpool 2 days ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.