Computing the item price and vat for a given pricing, order, and exchange rate

Question

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}]

Answer 1

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())

Answer 2

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?

Answer 3

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.

Answer 4

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()