6
\$\begingroup\$

How can I improve the readability of the below function using a list comprehension? Also, is there a way to improve items() performance?

pricing = {'prices': [{'product_id': 1, 'price': 599, 'vat_band': 'standard'},
                      {'product_id': 2, 'price': 250, 'vat_band': 'zero'},
                      {'product_id': 3, 'price': 250, 'vat_band': 'zero'}],
           'vat_bands': {'standard': 0.2, 'zero': 0}}
order = {'order': {'id': 12, 'items': [{'product_id': 1, 'quantity': 1}, {'product_id': 2,'quantity': 5}]}}
exchange_rate = 1.1

def items():
    """
    computes the item price and vat for a given pricing, order, and exchange rate.
    returns list of items dictionaries
    """
    return [{'product_id': item['product_id'],
             'quantity': item['quantity'],
             'price': round(product['price'] * exchange_rate, 2),
             'vat': round(pricing['vat_bands']['standard'] * product['price'] * exchange_rate, 2)}
            if product['vat_band'] == 'standard' else
            {'product_id': item['product_id'],
             'quantity': item['quantity'],
             'price': round(product['price'] * exchange_rate, 2),
             'vat': 0}
            for item in order['order']['items'] for product in pricing['prices']
            if item['product_id'] == product['product_id']]


print(items())

Output:

[{'product_id': 1, 'quantity': 1, 'price': 658.9, 'vat': 131.78},
 {'product_id': 2, 'quantity': 5, 'price': 275.0, 'vat': 0}]
\$\endgroup\$
5
  • 3
    \$\begingroup\$ This needs to be re-titled. What does the program actually do? Pricing for what? \$\endgroup\$
    – Reinderien
    Jul 1, 2020 at 15:33
  • 5
    \$\begingroup\$ Apart from the list comprehensions you asked for, Python dicts are not really meant to be used like this as far as I understand. In python, objects with properties are something different that dictionaries with keys; that's in contrast to JavaScript. You might want to use namedtuple or dataclass so that you have Products with prod.price instead of dicts with prod['price'] \$\endgroup\$ Jul 1, 2020 at 21:08
  • \$\begingroup\$ Personally the source of the readability issue seems to be mostly a whitespace issue. I found using hanging indents and keeping one expression per line to fix this. \$\endgroup\$
    – Peilonrayz
    Jul 2, 2020 at 0:23
  • \$\begingroup\$ The conditional expression is duplicating the entire dict when the only difference between the two cases is the vat value. \$\endgroup\$
    – chepner
    Jul 2, 2020 at 17:56
  • 1
    \$\begingroup\$ @SillyFreak That's basically what my answer proposes. \$\endgroup\$
    – Reinderien
    Jul 2, 2020 at 19:43

4 Answers 4

7
\$\begingroup\$

As a general rule it is best not to nest comprehensions. It makes code hard to read. You are better off just writing a for loop and appending the results to a list or using a generator.

Here are a couple rules with comprehensions that will make your code less brittle and easy for others to work with:

  1. Don't nest comprehensions.
  2. If a comprehension is too long for one line of code, don't use it.
  3. If you need an else, don't use a comprehension.

Of course there are exceptions to these rules, but they are a good place to start.

One of the reasons nested list comprehension is an issue is it often results in a exponential increase in computation needed. For each item in the order you have to loop through every product. This is not efficient. You want to go from O(n x m) to O(n + m). You should loop through products once and through order items once.

You can see in the updated code below that I loop through the list of products and create a dictionary with the key as the product ID. This makes it so that, while looping through the order items, I can simply get the product by looking up the key. It is much more performant and readable.

pricing = {
    "prices": [
        {"product_id": 1, "price": 599, "vat_band": "standard"},
        {"product_id": 2, "price": 250, "vat_band": "zero"},
        {"product_id": 3, "price": 250, "vat_band": "zero"},
    ],
    "vat_bands": {"standard": 0.2, "zero": 0},
}
order = {
    "order": {
        "id": 12,
        "items": [{"product_id": 1, "quantity": 1}, {"product_id": 2, "quantity": 5}],
    }
}
exchange_rate = 1.1


def calculate_exchange_rate(price, rate=None):
    if rate is None:
        rate = exchange_rate
    return round(price * rate, 2)


def items():
    """
    computes the item price and vat for a given pricing, order, and exchange rate.
    returns list of items dictionaries
    """
    item_list = []
    products = {p["product_id"]: p for p in pricing["prices"]}

    for item in order["order"]["items"]:
        product = products.get(item["product_id"])
        vat = 0
        if product["vat_band"] == "standard":
            vat = pricing["vat_bands"]["standard"] * product["price"]
        item_list.append(
            {
                "product_id": item["product_id"],
                "quantity": item["quantity"],
                "price": calculate_exchange_rate(product["price"]),
                "vat": calculate_exchange_rate(vat),
            }
        )
    return item_list

print(items())
\$\endgroup\$
6
  • 2
    \$\begingroup\$ I am bewildered by any discussion or proposal (including PEP8) that treats “readability” as a universal objectively definable standard. It is by its nature subjective. Honestly, I find nested list comprehensions more “readable” than nested for loops with an append() in the middle. Yes, like many people I grew up on plodding vanilla languages—C, pascal, BASIC—but to apply their standards to what is and is not “readable” in Python just demonstrates a failure to update one’s thinking. \$\endgroup\$
    – jez
    Jul 2, 2020 at 3:58
  • \$\begingroup\$ @jez Readability couldn't be 100% subjective though, there is something fundamental about the properties of information in regards to the readability by an agent. It's sometimes hard to get at since our experience and expectations shapes us so much, but I think it's a good endeavor to try to find the more objective bits here. \$\endgroup\$
    – Alex
    Jul 2, 2020 at 9:38
  • \$\begingroup\$ @Alex 100% subjectivity is a straw man—*of course* I’m not suggesting that. I’m objecting to the way the other extreme, i.e. pretense of 100% objectivity (e.g. slavish adherence to PEP8) is commonly advocated. This post, which declares “nested comprehensions are less readable than nested for loops, so don’t use them” is a good illustration of the subjectivity that really lies behind that. To me it’s the other way round, and I doubt I’m alone in the universe. \$\endgroup\$
    – jez
    Jul 2, 2020 at 12:47
  • 1
    \$\begingroup\$ @jez Then we agree, thank you for clarifying. \$\endgroup\$
    – Alex
    Jul 2, 2020 at 14:41
  • 1
    \$\begingroup\$ You forgot to return item_list. \$\endgroup\$
    – J.G.
    Jul 2, 2020 at 17:22
6
\$\begingroup\$

Readability

comprehensions have advantages

  • they may be short one liners that are more readable (in the context code) than explicit loops
  • they may be more efficient

however - when done wrong they tend to be unreadable and thus unmaintainable.

Yours is near unmaintainable. It did take me a little time to identify some of your code is superfluous. Your expression

return [{'product_id': item['product_id'],
         'quantity': item['quantity'],
         'price': round(product['price'] * exchange_rate, 2),
         'vat': round(pricing['vat_bands']['standard'] * product['price'] * exchange_rate, 2)}
        if product['vat_band'] == 'standard' else
        {'product_id': item['product_id'],
         'quantity': item['quantity'],
         'price': round(product['price'] * exchange_rate, 2),
         'vat': 0}
        for item in order['order']['items'] for product in pricing['prices']
        if item['product_id'] == product['product_id']]

contains a special handling for zero VAT - and your pricing does so as well. So we shorten the expression to

return [{'product_id': item['product_id'],
         'quantity': item['quantity'],
         'price': round(product['price'] * exchange_rate, 2),
         'vat': round(pricing['vat_bands'][product['vat_band']] * product['price'] * exchange_rate, 2)}
        for item in order['order']['items'] for product in pricing['prices']
        if item['product_id'] == product['product_id']]

Efficiency

Next the n * m loop. That is the most inefficient search. That is because your pricing data structure is not optimized for lookup. We solve that by converting the existing list to a dict once(!)

prices = {e['product_id']: {'price': e['price'], 'vat_band':e['vat_band']} for e in pricing['prices']}

That is what comprehensions are for mostly. We also do a shortcut for

vat_bands = pricing['vat_bands']

and have a simpler comprehension with a loop over orders only as we can directly look up pricing information

return [{'product_id': item['product_id'],
         'quantity': item['quantity'],
         'price': round(prices[item['product_id']]['price'] * exchange_rate, 2),
         'vat': round(vat_bands[prices[item['product_id']]['vat_band']] * prices[item['product_id']]['price'] * exchange_rate, 2)}
        for item in order['order']['items']]

More readability

We pull out some code into a function. That allows us to have temporary variables which add more readability.

pricing = {'prices': [{'product_id': 1, 'price': 599, 'vat_band': 'standard'},
                      {'product_id': 2, 'price': 250, 'vat_band': 'zero'},
                      {'product_id': 3, 'price': 250, 'vat_band': 'zero'}],
           'vat_bands': {'standard': 0.2, 'zero': 0}}
order = {'order': {'id': 12, 'items': [{'product_id': 1, 'quantity': 2}, {'product_id': 2,'quantity': 5}]}}
exchange_rate = 1.1

prices = {e['product_id']: {'price': e['price'], 'vat_band': e['vat_band']} for e in pricing['prices']}
vat_bands = pricing['vat_bands']


def do_format(item, product):
    price = round(product['price'] * exchange_rate, 2)
    vat = round(vat_bands[product['vat_band']] * product['price'] * exchange_rate, 2)
    return dict(item, **{'price': price, 'vat': vat})


def items():
    """
    computes the item price and vat for a given pricing, order, and exchange rate.
    returns list of items dictionaries
    """
    return [do_format(item, prices[item['product_id']]) for item in order['order']['items']]

Now everything is perfectly readable. So readable that we wonder why quantity has no effect on the price?

\$\endgroup\$
2
  • \$\begingroup\$ Don't know if it really matters, it's maybe less efficient if item is huge, but from the POV of readability I would write {**item, 'price': price, 'vat': vat} in preference to dict(item, **{'price': price, 'vat': vat}). Or if you don't like that, you can still tidy it up a bit as dict(item, price=price, vat=vat). \$\endgroup\$ Jul 1, 2020 at 21:53
  • \$\begingroup\$ @SteveJessop I did notice that only after posting. I agree with you. \$\endgroup\$
    – stefan
    Jul 2, 2020 at 7:17
5
\$\begingroup\$

Ignoring the fact that your particular code doesn't actually need a nested loop, in general I would say that nested comprehensions can be fairly readable, but helps a lot to put each for on a line of its own:

return [
    some_function_of(item, product)
    for item in order['order']['items']
    for product in pricing['prices']
    if some_condition_on(item, product)
]

The main issue with the way your code presents to the reader, is that the if/else clause is huge and the comprehension logic is tiny, so you can't easily see the structure of the comprehension itself until you've mentally eliminated the big if/else expression. This wouldn't be a problem if each logical part of the comprehension (the expression, the for clauses, the if clause) was small. If you can't achieve that, then a nested comprehension is going to be difficult to follow.

Alternatively, in a case like this where the "inner" nested for clause actually doesn't depend on the value of item from the outer for, you can also eliminate nesting using itertools.product:

return [
    some_function_of(item, product)
    for item, product in itertools.product(order['order']['items'], pricing['prices'])
    if some_condition_on(item, product)
]

Assuming the reader knows itertools, this has the advantage of immediately communicating that this is an N*M loop. Sometimes when reading nested comprehensions (or nested for loops for that matter), you spend a bit of time wondering in what way the inner loop bounds depend on the outer loop value: are we looking at a rectangle, or a triangle, or something wibbly? It's usually not particularly difficult to figure out when they are independent, but explicitly stating that this is a cartesian product eliminates the need to even think about that. Whenever you can show the reader the big structure first, that helps readability.

Then with that done we see that:

  • you're filtering a cartesian product
  • using a condition which by definition is only true for one pair per item
  • because (we hope) product_id uniquely identifies a product,

That is the clue that something is wrong here, and that it would be more efficient to look up the correct product for each item as in the other answers.

You may also notice that I'm using a different indenting style from you -- I put opening punctuation at the end of a line, and the matching closing punctuation at the start of a line, and I indent by fixed tabs rather than vertically matching the opening punctuation. That is to say, I use "hanging indents", and where PEP-8 says "The 4-space rule is optional for continuation lines", I choose to stick with 4! I think I'm probably in a minority of Python programmers who prefer this, though, so feel free to ignore it. Provided your indentation is reasonably consistent, it's only a minor contributor to readability which convention you follow.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ This is the answer I agree with the most. The comprehension itself is not the issue at all. The issue is in the conditional forming of the dictionary. \$\endgroup\$
    – Reinderien
    Jul 2, 2020 at 18:58
2
\$\begingroup\$

There's a lot of good feedback on how to work with the data as it's structured now, but my opinion is that - as soon as humanly possible - you should deserialize it out of a collection of weakly-typed dictionaries and lists, to a set of classes. This will make a handful of things better-structured, more testable and verifiable, and more easily maintainable and expandable. For instance, I added methods to calculate subtotals and print an "order table".

Also of note: please (please) do not round financials until the very last step of output. Doing otherwise is risking the wrath of accuracy loss, and in accounting that is indeed a bad place to be.

Example code:

from dataclasses import dataclass
from io import StringIO
from typing import Iterable, Dict, Tuple


EXCHANGE_RATE = 1.1


@dataclass
class Product:
    product_id: int
    price: float
    vat_band: float

    @classmethod
    def product_from_dict(cls, d: dict, bands: Dict[str, float]) -> 'Product':
        kwargs = {**d, 'vat_band': bands[d['vat_band']]}
        return cls(**kwargs)

    @classmethod
    def products_from_dict(cls, d: dict) -> Iterable['Product']:
        bands = d['vat_bands']
        return (
            cls.product_from_dict(price_d, bands)
            for price_d in d['prices']
        )

    @property
    def price_with_exchange(self) -> float:
        return self.price * EXCHANGE_RATE

    @property
    def vat_with_exchange(self) -> float:
        return self.vat_band * self.price_with_exchange

    @property
    def subtotal(self) -> float:
        return self.price_with_exchange + self.vat_with_exchange


@dataclass
class Item:
    product: Product
    qty: int

    @property
    def subtotal(self) -> float:
        return self.qty * self.product.subtotal


class Order:
    def __init__(self, d: dict, products: Dict[int, Product]):
        d = d['order']
        self.id = d['id']
        self.items: Tuple[Item] = tuple(
            Item(products[item['product_id']], item['quantity'])
            for item in d['items']
        )

    def __str__(self):
        out = StringIO()
        out.write(f'{"ID":2} {"Price":>6} {"VAT":>6} {"Qty":3} {"Subtotal":>6}\n')
        out.writelines(
            f'{item.product.product_id:2} '
            f'{item.product.price_with_exchange:6.2f} '
            f'{item.product.vat_with_exchange:6.2f} '
            f'{item.qty:3} '
            f'{item.subtotal:6.2f}\n'
            for item in self.items
        )
        return out.getvalue()


def main():
    products = {
        prod.product_id: prod
        for prod in Product.products_from_dict({
            'prices': [
                {'product_id': 1, 'price': 599, 'vat_band': 'standard'},
                {'product_id': 2, 'price': 250, 'vat_band': 'zero'},
                {'product_id': 3, 'price': 250, 'vat_band': 'zero'}],
            'vat_bands': {'standard': 0.2, 'zero': 0},
        })
    }
    order = Order({
        'order': {
            'id': 12, 'items': [
                {'product_id': 1, 'quantity': 1},
                {'product_id': 2, 'quantity': 5}
            ]
        }
    }, products)

    print(str(order))


if __name__ == '__main__':
    main()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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