4
\$\begingroup\$

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()
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! When you turn the device off, shouldn't the startDelay be added to the duration? (or rename duration to stopDelay) \$\endgroup\$
    – oliverpool
    Mar 1 '16 at 6:59
  • \$\begingroup\$ 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! \$\endgroup\$
    – Spencer
    Mar 2 '16 at 0:42
1
\$\begingroup\$

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__.

\$\endgroup\$
1
  • \$\begingroup\$ 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 :) \$\endgroup\$
    – Spencer
    Mar 2 '16 at 0:39
2
\$\begingroup\$

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!

\$\endgroup\$
2
  • \$\begingroup\$ Good point on only importing randint, thanks. SUPER cool about unpacking, I never knew about this! \$\endgroup\$
    – Spencer
    Mar 2 '16 at 18:20
  • 1
    \$\begingroup\$ Unpacking in python is very nice and can be used with almost all kind of collections (list, tuple...) \$\endgroup\$
    – oliverpool
    Mar 2 '16 at 20:55

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.