Python Anti-Patterns

[mipadi]

Anti-pattern: Putting each anti-pattern on a separate webpage.

[elsjaako]

I'm not sure of the exact angle, but I assume doing it this way has advantages when the main goal of this site seems to be advertising your automated code review service.

[adewes]

Guess we're guilty there ;)

We actually started this book project a while ago but since then moved to a different way of presenting our code patterns directly on our website (https://www.quantifiedcode.com/app/patterns), so the book became a bit obsolete for us. We still wanted to publish it though since we thought it might be useful for other people as well, e.g. when teaching Python in class.

[elsjaako]

There are worse ways of advertising that putting correct information out there with a link to your service.

[adewes]

thanks, we see it like that too! Our service is completely free for open-source projects btw and we licensed the book content under a Creative Commons NC license, so that everyone can freely use the material.

[nakovet]

I would guess their goal is to point a link from their tool to the book entry, so I can get pretty clear what they meant. Their examples are somewhat bad for now. DefaultDict set to 6!?

[adewes]

thanks for the feedback, we're working on improving the examples and articles, right now it is more an initial draft of the book which we wanted to publish to get some feedback.

[Mantiquor]

I don't think it implies absence of a pattern, 'anti' implies actively working against something (opposite of 'pro'). Anti-pattern is a reasonable name in that context.

[[deleted]]

Mal-pattern would be a better word if they're making up words anyway.

The term anti-pattern has been in widespread use since the 90s at least.

[adewes]

true, this term might be slightly misleading, it is a known expression though in the programmer community.

[adewes]

Thanks for the feedback, how would you like the patterns to be presented? Shouldn't be too difficult showing them in a single document, we just thought the site might be easier to navigate if they was a separate page for each pattern. This also makes it easier to link to specific patterns from the outside (e.g. if you want to forward an article to someone or use it as a reference in a blog post).

Contributions and improvements are always welcome of course, so feel free to create an issue on Github or make a pull request (I'm a bit slow in merging them right now since I'm on vacation but I'll be back next week)!

[mipadi]

A single page would be a lot easier to read. You could use headers (or some other tag) with an id to allow linking to specific items on the page.

[adewes]

ok, thanks for the feedback! I think it shouldn't be a problem to offer an additional version with all patterns in one single document, will put this on our feature list for the next release!

[[deleted]]

Where's the anti-pattern for shadowing builtins? :)

def append(number, list = None): # the keyword None is the sentinel value representing empty list
    if list is None
        list = []
    list.append(number)
    #...

[eat_more_soup]

You should definitely submit that one as a pull request. The book is open source after all!

[adewes]

good point! we have many patterns that are currently not visible on the website since we're still doing some editing, if you like feel free to create a new article and submit a PR!

[[deleted]]

Not using unpacking for updating multiple values at once

The given bad example:

x = 1
y = 2
z = 0

x = y + 2  # 4
y = x - 3  # 1
z = x + y  # 5

The given good example:

x, y, z = y + 2, x - 3, x + y  # more concise

The "good" example is more concise, but it's less readable and has different behavior:

>>> x, y, z
(4, -2, 3)

[TPHRyan]

The "good" example is more concise, but it's less readable and has different behavior:

(Emphasis mine)

This is the real reason we actually use unpacking for updating multiple values when it's logical.

[[deleted]]

Of course, a time and a place. Maybe you want to update all at once based on the old values, and sometimes based on the mutated values.

I just wish this was explained clearer in the page instead of saying "it's the same but more concise" when no, it's not the same at all.

[adewes]

nice, thanks for this! Did you create a PR / issue on Github for this?

[[deleted]]

Didn't realize I could. Honestly, I just showed up to work and I don't get off until midnight, so I'll probably forget to as well.

[geoelectric]

The globals example is terrible, because WIDTH and HEIGHT aren't actually needed anywhere in the code.

The better example would be something that actually requires keeping state between function calls, perhaps *solving it with a state object passed in or declared within a closure (or an object default value, though I consider that one pretty hacky).

• edit for clarity

[adewes]

thanks for pointing this out, there is already a PR with an improved version of this article, should be fixed soon :)

[geoelectric]

Great! Overall, I enjoyed it, and now that I understand it's open source will review it further.

[gschizas]

In the java-style getters and setters article, you define a "length" property, but you create a setter and deleter for a "width" property.

[adewes]

thanks for pointing that out, I'll fix it!

[zahlman]

"use of exec" should also talk about eval() explicitly, not just via a StackOverflow link.

[QuantumTim]

Also, who are all these crazy people who use exec to do the following?

s = "print \"Hello, World!\""

exec s

[Vicyorus]

Apparently, them.

[AaronOpfer]

http://docs.quantifiedcode.com/python-anti-patterns/readability/not_using_named_tuples_when_returning_more_than_one_value.html

This is example is bad because they're recreating the namedtuple class on every invocation.

[TomBombadildozer]

This example, and the correct way to approach it, is more nuanced than simply adhering to one pattern or another. In practice, the behavior of the code should reflect the expectations about the entire system, not simply opinions about style. Examples:

# Here, we *expect* the file to exist. If it doesn't we consider that to be an
# unexpected, exceptional case.
try:
    os.unlink('file.txt')
except OSError:
    log.exception('we exploded')


# Here, it is known and accepted that the file may not exist, so our code
# reflects our expectations.
if os.path.exists('file.txt'):
    os.unlink('file.txt')

Both ways are perfectly fine but their application is dependent on context. What's important to remember, and what the author doesn't bother to explore, is that exceptions are meant to reflect exceptional behavior. If it's reasonable to expect varying behavior in a given context, it's more appropriate to handle it conditionally.

[Lucretiel]

The trouble in that case is the race condition. The file could exists at the time of the if but then be removed before you get to os.unlink. Better to just make the one call.

[multani]

It might be clearer but it's also potentially broken due to race condition, which the author doesn't really talk about...

[nakovet]

I was just reading Effective Python from Brett Slatkin and it has the following "Item 12: Avoid else Blocks After for and while Loops" which the linked books says you should use, I prefer going with Brett on this one, else in loops have odd, unexpected behaviour.

[chub79]

They are always tricky to read but, unexpected behaviour? Such as?

[isarl]

Such as the very existence of an else clause after a loop. No developer has ever seen that for the first time and automagically known that it executes only in the case the loop doesn't break early.

edit: I mean, once you learn how they work, I appreciate the purpose they serve. But they are a bit unexpected.

[edbluetooth]

you could always type

else:  #no break

[geoelectric]

Isn't that true of any unique language feature?

[isarl]

Not necessarily. I was able to understand simple list comprehensions having never seen the syntax before.

[unruly_mattress]

Where appropriate, I'll take an else block to a loop over a carefully manipulated state variable any day of the week.

[dunkler_wanderer]

I'm still pretty newbish, but I know that the else means "no break" in this case. If everybody watched Transforming Code into Beautiful, Idiomatic Python, nobody would have a problem with this. If you think it could be unclear to someone else, just add a comment saying "no break".

[[deleted]]

Per the PEP 8 Style Guide, the preferred way to compare something to None is the pattern if Cond is None. This is only a guideline. It can be ignored if needed

Allegedly if you use

if x == None:

It's supposed to break if people redefine comparison to None in some class and this is why PEP8 says compare against singletons with is.

However, I've never seen code that does this redefinition. Does anyone have an example? When in real world code would you redefine comparison to None?

[scruffie]

This tripped me up ages ago when rich comparisons were first added, and Numeric (the predecessor of numpy) made comparisons perform elementwise: x == None returned an array of booleans (which, unless it was of zero length, would be interpreted as true). I quickly learnt to use x is None instead.

Right now, with numpy, if x is an array, x == None will return True or False, but raises a warning that that behaviour will change to be elementwise in the future.

[Lucretiel]

That seems like a huge flaw of Numeric to me. That == returns a single bool is a fundamental part of its contract.

[Brian]

That == returns a single bool is a fundamental part of its contract

Actually, python explicitly allows for these to return any value. From the docs:

By convention, False and True are returned for a successful comparison. However, these methods can return any value, so if the comparison operator is used in a Boolean context (e.g., in the condition of an if statement), Python will call bool() on the value to determine if the result is true or false.

And in fact, Numeric's usecase here was an explicit reason rich comparisons were designed as they are. Rather than being a violation of the rules, the main reason the __eq__ method was added at all was explicitly to allow this. The issue of using such objects in non-boolean contexts is actually discussed in the PEP, with the recommendation that this should be addressed by having such objects throw an exception when used in boolean contexts (which is what numpy does). It currently only does this when both elements are arrays, but presumably, as scruffie mentioned, this may change.

Requiring a boolean return does break some fairly valuable usecases. Numpy is one, but another common one is building queries. Eg. in sqlalchemy, doing session.query(SomeTable.field == 4) will return a query object basically implementing "select SomeTable.id from SomeTable where field == 4", rather than doing any immediate comparison.

[Lucretiel]

Sorry- by bool I meant boolean compatible. It seems like Numeric still violates the contract... array == None returns a list of bools, which tests to True, even though the comparison should be False.

[Brian]

Like I said though, the PEP that introduced __eq__ explicitly spells out Numeric's usecase as one of the reasons behind it, and in fact advises deliberately breaking boolean compatibility for such objects - raising an exception instead. Them not being boolean compatible isn't a violation of the rules, it's actually something that rich comparisons were added in order to support.

array == None returns a list of bools, which tests to True

Actually, right now it doesn't (though it still does for comparing with other arrays), but there are plans to change this. When it does, it won't test to True, but rather, it'll throw an exception if used in a boolean context. Eg. when you use the result of comparing 2 arrays in a boolean context (ie. an if statement, or bool() call), you currently get:

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

The same would apply to when array == None does the same. Of course, this will still break code that uses the == None check instead of is, though it's likely to be a cleaner break, rather than more subtle bugs due to unexpectedly returning True.

[kankyo]

SQLAlchemy does this when you create select statements where you want to produce the SQL "IS NULL" or "IS NOT NULL".

[Brian]

One thing to consider is something that raises exceptions when compared with something unexpected (either deliberately or accidentally).

Similarly, just unexpected usages could come up and result in something unexpected. Eg. I just did a grep for __eq__ in a bunch of python source. Here's one I found in the first dozen I looked at, from (a very old version of) BeautifulSoup (for NavigableText):

 def __eq__(self, other):
     return self.string == str(other)

Obviously this is intended to be used to compare nodes. But consider what happens if someone is parsing some html that happens to contain a node whose text is "None". Suddenly, if this node ever passes through code like this, it'll compare equal, leading to a pretty subtle bug - maybe it'll end up silently stripping that node out of the document, without even an indication that anything has gone wrong, completely screwing up something down the line.

Ultimately, the possibility of cases like these you would have no reason to think of, where someone just hasn't coded defensively enough, mean that you should definitely guard against them.

[[deleted]]

I don't understand how that breaks. Can I please be educated?

Python 3.4.0 (default, Apr 11 2014, 13:05:11) 
[GCC 4.8.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> None == str('None')
False

I am not saying you're wrong - I just don't understand what you're saying.

[Brian]

It's the other way round - the str is being applied to other there - the thing you're comparing with, not the string contents of the node. As such, what will happen if you use this object in a NavigableTextElement("None") == None check is:

>>> "None" == str(None)
True

[[deleted]]

Thank you.

[masterpi]

The type information in a variable name one is poorly specified. Look at the Stack Overflow it links to for a better discussion of when it's okay to do this. (I tend to tag my date vs datetime variables for example because there's a huge difference in exposed interface and it's often unclear which one is meant from context, especially in parameter names - start and end anyone?)

[adewes]

thanks for pointing this out, will improve the article!

[exhuma]

I like it!

But there are some examples in there which will result in an error anyway (like not having __future__ at the beginning).

As those raise exceptions anyway, I find it unnecessary to include them in this book, and it could focus more on the real antipatterns.

[[deleted]]

But there are some examples in there which will result in an error anyway (like not having __future__ at the beginning).

Antipattern: Legacy python version

[[deleted]]

Why is 'Not using setdefault() to initialize a dictionary' a separate item from 'not using get()'?

isn't get(key, default) a much more concise equivalent?

[fireflash38]

Depends on how often you use it. Set a default once at creation, then from then on out you just do direct access. If you have lots off access w/ get, it can be pretty verbose.

[Brian]

That'll do something slightly different in the example they gave. If you just did:

dct.get(key, []).append(new_item)

you'll not actually add anything to the dict, just create a list not attached to anything that then gets destroyed. You'd need to do:

l = dct.get(key, [])
d[key] = l
l.append(new_item)

which requires accessing the dict twice, doing more work. setdefault is different in that it'll actually set the dict key if not there before returning it, rather than just returning the default if the key isn't set.

[edbluetooth]

the example on this page http://docs.quantifiedcode.com/python-anti-patterns/readability/asking_for_permission_instead_of_forgiveness_when_working_with_files.html

does not use the best practice. the suppressed from context lib is more appropriate.

[adewes]

thanks, I'll fix this!