6
\$\begingroup\$

I'm shopping for LED strips and as there are so many variations I need some help by visualizing different product properties.

For that I made a class that describes(holds data for) LED strips. There are many parameters and many can be derived from each other. To initiate the class I will enter the data that is presented on online store and let the class automatically fill in the rest.
The entered data will always be sufficient to calculate all other required parameters, so there is no need to handle corner cases.

My implementation of the class however seems bit clumsy, confusing and trying to add new parameters is a nightmare. There has to be a better way.
The most important things for me in this case are: clarity > pythonic style > code brevity > execution speed.

class LED:
    _required_fields = {'watt', 'watt_meter', 'lumen', 'lumen_meter', 'lumen_watt',
                        'length', 'price', 'price_meter', 'price_watt', 'price_lumen'}

    def __init__(self, colour=None, watt=None, watt_meter=None, lumen=None,
                 lumen_meter=None, lumen_watt=None, length=None, price=None,
                 price_meter=None, price_watt=None, price_lumen=None,
                 seller=None, producer=None, url=None, note=None):
        self.colour = colour
        self.watt = watt
        self.watt_meter = watt_meter
        self.lumen = lumen
        self.lumen_meter = lumen_meter
        self.lumen_watt = lumen_watt
        self.length = length
        self.price = price
        self.price_meter = price_meter
        self.price_watt = price_watt
        self.price_lumen = price_lumen
        self.seller = seller
        self.producer = producer
        self.url = url
        self.note = note

        self.calculate_missing_values()

    def calculate_missing_values(self):
        """
        Iterate over all required fields and assume they start with no value.
        Check if field has value, if no then try to calculate value for field.
        If field had value or calculation was successful remove the field from
        empty_fields list.
        Repeat with remaining empty fields.


        Each required field has corresponding __calculate_<field>__(self) method that knows
        how to calculate it. Primitive fields (watt, lumen, length, price) are calculated
        first, the ratios of them (ie. lumen/watt) are only calculated when both are
        known (even though it might be possible to calculate them sooner)
        """
        empty_fields = self._required_fields

        while empty_fields:
            existing_values = set()
            for field in empty_fields:
                # Sometimes given data is conflicting, don't recalculate known values
                if not getattr(self, field):
                    getattr(self, f'__calculate_{field}__')()
                value = getattr(self, field)
                if value is not None:
                    existing_values.add(field)
            empty_fields = empty_fields.difference(existing_values)

    def __calculate_watt__(self):
        if self.watt_meter is not None and self.length is not None:
            self.watt = self.watt_meter * self.length
        elif self.lumen_watt is not None and self.lumen is not None:
            self.watt = self.lumen / self.lumen_watt
        elif self.price_watt is not None and self.price is not None:
            self.watt = self.price / self.price_watt

    def __calculate_lumen__(self):
        if self.lumen_watt is not None and self.watt is not None:
            self.lumen = self.lumen_watt * self.watt
        elif self.lumen_meter is not None and self.length is not None:
            self.lumen = self.lumen_meter * self.length
        elif self.price_lumen is not None and self.price is not None:
            self.lumen = self.price / self.price_lumen

    def __calculate_length__(self):
        if self.lumen_meter is not None and self.lumen is not None:
            self.length = self.lumen / self.lumen_meter
        elif self.watt_meter is not None and self.watt is not None:
            self.length = self.watt / self.watt_meter
        elif self.price_meter is not None and self.price is not None:
            self.length = self.price / self.price_meter

    def __calculate_price__(self):
        if self.price_meter is not None and self.length is not None:
            self.price = self.price_meter * self.length
        elif self.price_lumen is not None and self.lumen is not None:
            self.price = self.price_lumen * self.lumen
        elif self.price_watt is not None and self.watt is not None:
            self.price = self.price_watt * self.watt

    def __calculate_watt_meter__(self):
        if self.length is not None and self.watt is not None:
            self.watt_meter = self.watt / self.length

    def __calculate_lumen_meter__(self):
        if self.length is not None and self.lumen is not None:
            self.lumen_meter = self.lumen / self.length

    def __calculate_lumen_watt__(self):
        if self.watt is not None and self.lumen is not None:
            self.lumen_watt = self.lumen / self.watt

    def __calculate_price_meter__(self):
        if self.price is not None and self.length is not None:
            self.price_meter = self.price / self.length

    def __calculate_price_watt__(self):
        if self.price is not None and self.watt is not None:
            self.price_watt = self.price / self.watt

    def __calculate_price_lumen__(self):
        if self.price is not None and self.lumen is not None:
            self.price_lumen = self.price / self.lumen

    def __str__(self):
        return f'Colour: {self.colour} K,\t\t\t' \
               f'Length: {self.length:1.0f} m,\t\t\t' \
               f'Power: {self.watt:\040>3.0f} W, {self.watt_meter:\040>4.1f} W/m,\t\t\t' \
               f'Illuminance: {self.lumen:\040>5.0f} lm, {self.lumen_meter:\040>4.0f} lm/m, {self.lumen_watt:\040>3.0f} lm/W,\t\t\t' \
               f'Price {self.price:\040>5.2f} €, {self.price_meter:\040>5.2f} €/m, {self.price_watt:\040>5.3f} €/W, {self.price_lumen*1000:\040>6.3f} m€/lm,\t\t\t' \
               f'Producer: {self.producer if self.producer else "None":\040>10},\t\t\t'\
               f'Url: {self.url}'

    def __repr__(self):
        return self.__str__()

Example usage:

led_strip_a = {
    'watt': 15,
    'lumen_meter': 1000,
    'price': 15.60,
    'length': 5,
}

led_strip_b = {
    'watt': 15,
    'lumen_watt': 333,
    'price_lumen': 0.00312,
    'price_meter': 3.12,
}

print(LED(**led_strip_a))
print(LED(**led_strip_b))

Output:

Colour: None K,         Length: 5 m,            Power:  15 W,  3.0 W/m,         Illuminance:  5000 lm, 1000 lm/m, 333 lm/W,         Price 15.60 €,  3.12 €/m, 1.040 €/W,  3.120 m€/lm,          Producer:       None,           Url: None
Colour: None K,         Length: 5 m,            Power:  15 W,  3.0 W/m,         Illuminance:  4995 lm, 1000 lm/m, 333 lm/W,         Price 15.58 €,  3.12 €/m, 1.039 €/W,  3.120 m€/lm,          Producer:       None,           Url: None
\$\endgroup\$
4
\$\begingroup\$

Repetitiveness

All of your calculations are variants of six formulas:

  • lumen = lumen_meter * length
  • lumen = lumen_watt * watt
  • price = price_lumen * lumen
  • price = price_meter * length
  • price = price_watt * watt
  • watt = watt_meter * length

Therefore, it would be ideal to represent those formulas in your code just once, perhaps like this:

FORMULAS = [
    ProportionalFormula('lumen', 'lumen_meter', 'length'),
    ProportionalFormula('lumen', 'lumen_watt', 'watt'),
    ProportionalFormula('price', 'price_lumen', 'lumen'),
    ProportionalFormula('price', 'price_meter', 'length'),
    ProportionalFormula('price', 'price_watt', 'watt'),
    ProportionalFormula('watt', 'watt_meter', 'length'),
]

See the suggested solution below for an example of how to make that work.

Going further, you could even infer those formulas in general, without specifying them explicitly, just based on the variable names. You mix the unit ("meter") with the quantity ("length") in some cases, though, which makes it impractical.

Minor bugs

If the inputs are underspecified, calculate_missing_values() will enter an infinite loop. You should detect when no progress is being made, and raise an exception.

In calculate_missing_values(), take care to distinguish between 0 and None:

# Sometimes given data is conflicting, don't recalculate known values
if not getattr(self, field):

The former should represent a valid value; the latter represents an unknown. Both are falsy, and are treated the same in the test above.

Other remarks

Don't write __double_underscore__() methods where you don't mean it. Don't override __repr__() with arbitrary human-friendly strings.

I would design the class to distinguish between given and inferred values. At the least, it could make debugging easier, especially if the code is incorporated into a larger program.

_required_fields is a constant, so I would name it using ALL_CAPS. I would duplicate it once at the beginning with empty_fields = set(self._required_fields), then update it using empty_fields.difference_update(…) thereafter.

In __str__(), you can simplify f'Producer: {self.producer if self.producer else "None":\040>10} as f'Producer: {self.producer or "None": >10}.


Suggested solution

from collections import namedtuple

class ProportionalFormula(namedtuple('ProportionalFormula',
                                     'product factor1 factor2')):
    """
    A formula that represents a "product = factor1 * factor2" relationship.
    """

    def calculate(self, want, knowns):
        """
        Calculate the wanted variable based on attributes retrieved from the
        knowns object using getattr(), if possible.  Return None if this
        formula is irrelevant or if not enough of the variables have known
        values.
        """
        def value(field): return getattr(knowns, field)
        def known(field): return value(field) is not None
        def need(field): return field == want
        if need(self.product) and known(self.factor1) and known(self.factor2):
            return value(self.factor1) * value(self.factor2)
        if known(self.product) and known(self.factor1) and need(self.factor2):
            return value(self.product) / value(self.factor1)
        if known(self.product) and need(self.factor1) and known(self.factor2):
            return value(self.product) / value(self.factor2)
        return None

    def __str__(self):
        return f'{self.product} = {self.factor1} * {self.factor2}'

class LED:
    ESSENTIAL_FIELDS = {
        'watt', 'watt_meter', 'lumen', 'lumen_meter', 'lumen_watt', 
        'length', 'price', 'price_meter', 'price_watt', 'price_lumen',
    }
    FORMULAS = [
        ProportionalFormula('lumen', 'lumen_meter', 'length'),
        ProportionalFormula('lumen', 'lumen_watt', 'watt'),
        ProportionalFormula('price', 'price_lumen', 'lumen'),
        ProportionalFormula('price', 'price_meter', 'length'),
        ProportionalFormula('price', 'price_watt', 'watt'),
        ProportionalFormula('watt', 'watt_meter', 'length'),
    ]

    def __init__(self, colour=None, watt=None, watt_meter=None, lumen=None,
                 lumen_meter=None, lumen_watt=None, length=None, price=None,
                 price_meter=None, price_watt=None, price_lumen=None,
                 seller=None, producer=None, url=None, note=None):
        args = locals()
        self._given, self._inferred, unknowns = {}, {}, set()

        # Set the given and unknowns based on parameters
        for field in self.ESSENTIAL_FIELDS:
            if args[field] is not None:
                self._given[field] = args[field]
            else:
                unknowns.add(field)

        # Supplementary information, not used in calculations
        self.colour = colour
        self.seller = seller
        self.producer = producer
        self.url = url
        self.note = note

        # Calculate the unknowns
        while unknowns:
            calcs = {field: self._calculate(field) for field in unknowns}
            calcs = {
                field: value for field, value in calcs.items()
                if value is not None
            }
            if not calcs:
                raise KeyError('Cannot calculate ' + ', '.join(unknowns))
            for field, value in calcs.items():
                self._inferred[field] = value
            unknowns.difference_update(set(calcs.keys()))

    def __getattr__(self, field):
        return self._given.get(field, self._inferred.get(field))

    def _calculate(self, field):
        """
        Calculate the specified field based on existing given or inferred
        values.  Return None if none of the FORMULAS provides a way to
        calculate the value.
        """
        for formula in self.FORMULAS:
            value = formula.calculate(field, self)
            if value is not None:
                return value


    def __str__(self):
        return (
            f'Colour: {self.colour} K,\t\t\t'
            f'Length: {self.length:1.0f} m,\t\t\t'
            f'Power: {self.watt: >3.0f} W,'
            f' {self.watt_meter: >4.1f} W/m,\t\t\t'
            f'Illuminance: {self.lumen: >5.0f} lm,'
            f' {self.lumen_meter: >4.0f} lm/m,'
            f' {self.lumen_watt: >3.0f} lm/W,\t\t\t'
            f'Price {self.price: >5.2f} €,'
            f' {self.price_meter: >5.2f} €/m,'
            f' {self.price_watt: >5.3f} €/W,'
            f' {self.price_lumen*1000: >6.3f} m€/lm,\t\t\t'
            f'Producer: {self.producer or "None": >10},\t\t\t'
            f'Url: {self.url}'
        )
\$\endgroup\$
  • \$\begingroup\$ Ok, so if I implement repr I should make sure eval() of that will return the same object? (which it doesn't for default implementation). (I heard from somewhere, long time ago, that single underscore is for methods that others shouldn't use, and double for when they really really shouldn't use it. Never been so wrong. Good I don't write python that others have to use :) \$\endgroup\$ – Kristjan Kiolein Mar 7 '18 at 10:31
  • \$\begingroup\$ For some reason, if I set break point on this line : args = locals() (also on next line, but no other line does it) I get this error: Fatal Python error: Cannot recover from stack overflow. Otherwise love the ProportionalFormula, no information duplication at all! \$\endgroup\$ – Kristjan Kiolein Mar 7 '18 at 11:23
1
\$\begingroup\$

Your code looks very nice and is well documented. Also the intent to avoid boilerplate code while keeping it understandable is both notable and successful. Here are a few details I would change anyway.

Consistency

For some reason, this particular piece of code

           if not getattr(self, field):
                getattr(self, f'__calculate_{field}__')()
            value = getattr(self, field)
            if value is not None:

bugged more than it should have. The reason is that you have a similar-looking logic in 2 places which is not exactly the same. Once you get getattr(self, field) and check if the result is False (or falsy) and the second time you check if the result is None. Both operations have often the same result but not always: None is considered falsy but is not the only falsy value - the most common falsy values are False, 0, "", [], etc.

Anyway, in your case, an unset field has the None value so it is probably best to use is None (or is not None) in both cases to avoid resetting a value computed and equal to 0.

Then, things look a bit more consistent:

            if getattr(self, field) is None:
                getattr(self, '__calculate_%s__' % field)()
            if getattr(self, field) is not None:
                existing_values.add(field)

Making things more explicit

From The Zen of Python:

Explicit is better than implicit.

I think in your case, it would be more explicit to define a dictionnary mapping field names to function being called to set these fielde. It would make the code easier to search in. My first idea going too quickly over your code was "these functions are called nowhere", I was obviously wrong but you can see why I thought so.

This could be done via the not-so-beautiful:

    empty_fields = {
         'watt': self.__calculate_watt__,
         'watt_meter': self.__calculate_watt_meter__,
         'lumen': self.__calculate_lumen__,
         'lumen_meter': self.__calculate_lumen_meter__,
         'lumen_watt': self.__calculate_lumen_watt__,
         'length': self.__calculate_length__,
         'price': self.__calculate_price__,
         'price_meter': self.__calculate_price_meter__,
         'price_watt': self.__calculate_price_watt__,
         'price_lumen': self.__calculate_price_lumen__,
    }

    while empty_fields:
        existing_values = set()
        for field, method in empty_fields.items():
            # Sometimes given data is conflicting, don't recalculate known values
            value = getattr(self, field)
            if value is None:
                method()
            value = getattr(self, field)
            if value is not None:
                existing_values.add(field)
        empty_fields = {k:v for k, v in empty_fields.items() if k not in existing_values}

Distinction between computing a value and setting the field

It may be interesting to make a distinction between functions computing values and functions setting the different fields. It would make things less verbose in the various small functions and more explicit in the calculate_missing_value functions.

For instance:

    while empty_fields:
        existing_values = set()
        for field, method in empty_fields.items():
            # Sometimes given data is conflicting, don't recalculate known values
            value = getattr(self, field)
            if value is None:
                value = method()
                setattr(self, field, value)
            if value is not None:
                existing_values.add(field)
        empty_fields = {k:v for k, v in empty_fields.items() if k not in existing_values} 


def __calculate_watt__(self):
    if self.watt_meter is not None and self.length is not None:
        return self.watt_meter * self.length
    elif self.lumen_watt is not None and self.lumen is not None:
        return self.lumen / self.lumen_watt
    elif self.price_watt is not None and self.price is not None:
        return self.price / self.price_watt
    return None

Less verbose computing functions

All the small functions have a similar logic: keep trying to apply an operator to 2 values if they are not None. Because of how things are implemented, it is easy to get wrong (you can copy-paste someting and update the check or not the operations). It is also tedious to add a conversion because of the amount of boilerplate. This can be written in a more concise way with a minimal amount of magic.

With:

import operator
mult = operator.mul
divi = operator.truediv

def try_operations(operations):
    for (func, op1, op2) in operations:
        if op1 is not None and op2 is not None:
            return func(op1, op2)
    return None

You could have:

def __calculate_watt__(self):
    ops = [(mult, self.watt_meter, self.length),
           (divi, self.lumen, self.lumen_watt),
           (divi, self.price, self.price_watt)]
    return try_operations(ops)

def __calculate_lumen__(self):
    ops = [(mult, self.lumen_watt, self.watt),
           (mult, self.lumen_meter, self.length),
           (divi, self.price, self.price_lumen)]
    return try_operations(ops)

def __calculate_length__(self):
    ops = [(divi, self.lumen, self.lumen_meter),
           (divi, self.watt, self.watt_meter),
           (divi, self.price, self.price_meter)]
    return try_operations(ops)

If you want to go further, you could have all the relationships in a single place.

RELATION_BETWEEN_FIELDS = {
     'watt':        [(mult, 'watt_meter',  'length'),
                     (divi, 'lumen',       'lumen_watt'),
                     (divi, 'price',       'price_watt')],
     'watt_meter':  [(divi, 'watt',        'length')],
     'lumen':       [(mult, 'lumen_watt',  'watt'),
                     (mult, 'lumen_meter', 'length'),
                     (divi, 'price',       'price_lumen')],
     'lumen_meter': [(divi, 'lumen',       'length')],
     'lumen_watt':  [(divi, 'lumen',       'watt')],
     'length':      [(divi, 'lumen',       'lumen_meter'),
                     (divi, 'watt',        'watt_meter'),
                     (divi, 'price',       'price_meter')],
     'price':       [(mult, 'price_meter', 'length'),
                     (mult, 'price_lumen', 'lumen'),
                     (mult, 'price_watt',  'watt')],
     'price_meter': [(divi, 'price',       'length')],
     'price_watt':  [(divi, 'price',       'watt')],
     'price_lumen': [(divi, 'price',       'lumen')],
}

then:

    empty_fields = dict(RELATION_BETWEEN_FIELDS)
    while empty_fields:
        existing_values = set()
        for field, operations in empty_fields.items():
            # Sometimes given data is conflicting, don't recalculate known values
            value = getattr(self, field)
            if value is None:
                for op, field1, field2 in operations:
                    v1, v2 = getattr(self, field1), getattr(self, field2)
                    if v1 is not None and v2 is not None:
                        value = op(v1, v2)
                        setattr(self, field, value)
                        break
            if value is not None:
                existing_values.add(field)
        empty_fields = {k:v for k, v in empty_fields.items() if k not in existing_values}

and no more small functions.

A more Pythonic solution/option

Using properties, you may be able to implement a solution computing values when you try to access members and they are not set yet.

\$\endgroup\$
  • \$\begingroup\$ You can write from operator import mul as mult, truediv as divi. \$\endgroup\$ – 200_success Mar 6 '18 at 22:17
  • \$\begingroup\$ That's a good point. If i would be looking this code and Ctrl-click on the methods and my IDE tells me they are not used anywhere I could think I can just delete them. \$\endgroup\$ – Kristjan Kiolein Mar 7 '18 at 10:30

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.