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?
Product
s withprod.price
instead ofdict
s withprod['price']
\$\endgroup\$dict
when the only difference between the two cases is thevat
value. \$\endgroup\$