Dan Bader

A Python refactoring gone wrong

Ever witnessed a colleague make a refactoring to “clean up” some Python code only to make it worse and harder to understand?

I know I did. And I’ve also been that colleague to others many times 😊

There’s often a fine line between making code better by “cleaning it up” and just shuffling around or even making it slightly worse. Refactoring is hard!

It’s challenging to come up with good examples for this – so I was delighted when I got this Python question from Bev:

I came across something in Python that I’m having difficulty understanding. As part of a code rewrite in a Python related Youtube video, an “if” statement was changed to:

if any([self.temperature > MAX_TEMPERATURE,
        self.pressure > MAX_PRESSURE]):

Why was that used rather than the simpler:

if (self.temperature > MAX_TEMPERATURE
    or self.pressure > MAX_PRESSURE):

Why create a list and call a function in the if statement when there are only two (2) comparisons?

I agree with Bev, this is a surprising change and I don’t think it’s for the better!

It complicates the code for no apparent gain.

Let’s take a look at the definition for the any() function first:

Return True if any element of the iterable is true. If the iterable is empty, return False (Source: Python docs)

any() – and its colleague, all() – are handy if you need to check the elements of an iterable (like a list or a generator) for truthiness.

In some cases using any() can help avoid having to write a loop to check the elements of the iterable individually1. Imagine you needed to do something like this:

result = False
for elem in my_iterable:
    if elem:
        result = True

You could replace these 5 lines with a simple assignment using the any() function:

result = any(my_iterable)

In this case it makes sense to go with the any() solution because it is shorter and easier to understand.

Yet, too much of a good thing can make you sick… 😷

In my mind it doesn’t make much sense to use any() if all you have is a fixed size list with 2-3 elements, like in the example Bev found:

  • Constructing a list so we have an iterable to pass to any() is confusing. It adds visual clutter and is a non-standard pattern.

  • On top of that, using any() is also going to be slower than a nice and simple “or” expression: The Python interpreter first needs to construct that list and then call any() on it2.

In summary, I think this refactoring was not helpful. I’d say it actually made the code worse by making it slightly harder to read and understand.

As developers we need to be careful and deliberate about using the tools and patterns we know. “Fancy” often won’t equal “better”.

Great question, Bev! I think you were spot-on in doubting this refactoring 😃

  1. It’s good to note that any() has short-circuit evaluation semantics, so using it is equivalent to a chain of or operations. (Thanks to Lev Maximov for his feedback.) 

  2. In all fairness, this won’t make a big real-world difference in performance or memory usage – but it’s always good to keep these things in mind. 

Your Shortcut to a Productive Python Setup with Sublime Text: Get a professional Sublime Text setup for writing Python that makes you more productive and will be an absolute joy to use. » Click here to learn more

Improve Your Python with a fresh 🐍 Python Trick 💌 every couple of days

🔒 No spam ever. Unsubscribe any time.

This article was filed under: code-review, craftsmanship, programming, and python.

Related Articles:
Latest Articles:

📘 Write Cleaner & More Pythonic Code: Discover Python’s best practices with simple examples and start writing even more beautiful + Pythonic code. My book Python Tricks will show you how: » Click here to get a free sample chapter

← Browse All Articles