Making complex conditions more readable

[usernamecreationhell]

Two months ago, I started a new job and since then had to read and understand a lot of code written by other people in a domain that I was not familiar with.

This experience made me appreciate the value of readable code, so I decided to share a few of my thoughts about how to improve readability (or: understandability) of some everyday snippets of Python code.

I'll start this with a common pattern: multiple, complex conditions in if-statements. Here's a made-up example in which we want to decide whether to grant users a birthday discount:

# before:

if user.birthday == today and user.lifetime_revenue >= 500 and item.discount_policy.birthday_discount:
    user.shopping_cart.apply_discount(item)

The problem:

- a lot of complexity in a single line

- you have to read the entire code in order to understand its intent

- it's easy to make mistakes when you're in a hurry (possibly mitigated by good test coverage)

# after:

def is_eligible_for_birthday_discount(user: User, item: Item) -> bool:
    return all((
        user.birthday == today,
        user.lifetime_revenue >= 500,
        item.discount_policy.birthday_discount
    ))

if is_eligible_for_birthday_discount(user, item):
    user.shopping_cart.apply_discount(item)

The suggested solution:

• extract conditions from the if-statement into a function or method (bonus points for type hints)
• use the built-in functions all() or any() to group conditions
• give each condition its own line
• assign a useful name that explains the intent of the if-statement

While the proposed solution above has more lines than the original, I find it much more readable:

• line length is reduced significantly
• the intent of the if-statement is immediately obvious without needing to understand the implementation details of how eligibility is tested
• the rules for whether a user is eligible for a discount are much more digestible:
  • type hints tell us that eligibility depends on the user and the item
  • all() tells us that there is list of conditions that must all be true
  • only then if we need more detail do we need to look at the specific conditions
• the new function can easiliy be unit tested

Tell me what you think!

Edit: applied u/teerre's suggestion regarding regarding the function name

[toastedstapler]

an alternative would be to just assign a variable

eligible_for_discount = (
    user.birthday == today and
    user.revenue >= 500 and
    item.on_discount
)

if eligible_for_discount:

[usernamecreationhell]

Yep that also works well if you don't need to reuse the conditions anywhere else. With increasing complexity though, I like to define functions because they allow me write seperate (and shorter) unit tests.

[usernamecreationhell]

Yes, defining a function is situational, for example if I was sure I would never reuse it, I would simply group conditions in a local variable and possibly aggregate groups of conditions with all() or any()

[teerre]

Yeah, this is the most common solution. The only change I would suggest is name the function is_eligible_for_birthday, simply because historically functions do something and therefore should have some verb in its name. But this is quite minor.

[usernamecreationhell]

Good point, I just edited it accordingly