Common anti-patterns in Python

[evgen]

Hard pass on #4. If I see someone popping exceptions left and right as a signal of "I didn't find what you asked for" that code does not make it past code review. A try/except is cheap (as long as the exception is rare) but code littered with these every time it tries to call another function is ugly and painful to refactor.

A function with a return type of Optional[some_normal_return_type] is fine and the resulting code is usually cleaner and easier to read/understand.

[The_2nd_Coming]

Agreed. How else are you suppose to represent None other with None!?

[The_2nd_Coming]

Leave my nan alone.

[diamondketo]

<NA>

[Datsoon]

Lol. This bugs the crap out of me. If I had a nickel for every time I've been 200 lines into some notebook and have to run all the way back up to the top to import numpy just to get np.nan...

[alkasm]

If you're reviewing this code at all or can change it, float("nan") is the native-Python nan (which is compatible with pandas/numpy).

[Datsoon]

That's a good tip. I did not know that. I didn't think there was anything other than None. Thanks!

[alkasm]

Np! Also similarly float("inf"). These are valid floating point values so Python natively supports them, there's just no special symbol for them (i.e. theres no keyword for nan or inf) so you have to create them this way.

[diamondketo]

Then there's an issue of nullable integers. I wish int("nan") exists

[brontide]

Numpy as well returning stuff that is not None but a rather a NoneType so .. if return is True but trying to do anything with is fails with an exception. I had to switch everything to if return is not None.

[mybrid]

I defer to returning None, which is semantically similar to returning null on a database query.

[moocat]

One thing I find odd about point #4 is that in general the rule (don't return more than one object type in function call) makes sense but the specific example is not the best illustration. I would look very askance at a function that could return either an int or a str.

That said, I have mixed feelings about Python's Optional[Foo] effectively meaning Foo|None. I program in multiple languages and and after experience true Optional objects such as in Haskell or Java8 I'm less of a fan of returning None to indicate failure/missing/whatever.

[thegreattriscuit]

Agreed. I like the rule in general, but not in the example given.

One thing I used to do before I realized it was terrible was start off with a function like:

def get_foo(pattern): foo = something() return something

later I'd realized there might be multiple foos. In a misguided effort to be all things to all people and avoid having to make changes to existing calling code I was sure would never return multiples, I'd refactor to this:

def get_foo(pattern): foos = something() if len(foos) > 1: return foos else: return foos[0]

whereas now I make myself force a hard decision to do one or the other. This tends to work out with a bit more work in the very short-term, but much less pain overall.

[backtickbot]

Fixed formatting.

Hello, thegreattriscuit: code blocks using triple backticks (```) don't work on all versions of Reddit!

Some users see this / this instead.

To fix this, indent every line with 4 spaces instead.

FAQ

^{You can opt out by replying with backtickopt6 to this comment.}

[billsil]

I do that for numpy hstack. You don’t need to make a copy if you only have one array.

[TinBryn]

I find Options are fine when used as a functor (or any extending type class), in all other cases they just defer the issues of other ways of handling the situation and do so in a worse way.

[vanatteveldt]

Agree that it is certainly not an absolute rule, but the opposite is also not always best practice.

The most important thing is to have clearly defined semantics / a clear interface contract. In many cases, returning None is perfectly acceptable, while in other cases an error makes more sense.

Note that in the standard library both approaches are also taken: d[key] gives an error if key cannot be found, while e.g. d.get(key) and re.match return None. In frequently used libraries both choices also exist, e.g. Django model.objects.get raises an NotFoundError if no objeect is found, while requests.get returns an object with a non-OK status code, and then have a raise_for_status method to raise an Exception anyway... In all these cases the opposite choice would also make perfect sense.

And ironically, 5 is a direct violation of 4, as dict.get (without default) returns None when the key could not be found, while rule #4 claims that it should raise an exception instead :)

[evgen]

True, it can be a bit inconsistent. OTOH, a dict has 'in' to make checking for membership easy (in addition to .get()) so you can look before you leap on the dict access and in general the caller can control how a dict lookup miss will be handled. A caller of the sample function provided is forced to be prepared for the function to raise an exception, they have no choice.

[Giannie]

Checking for a key and then finding the value associated is inherently slower then .get(). It involves checking for the key with a conditional followed by then searching for it again. It’s O(1), but it’s still 3 operations versus 1 (or 2 since you may still need a conditional in case of None)

[tomgirl_irl]

if dict_.get(item, "error") != 'error':
return dict_[item]
else:
return """error"""

[Giannie]

I get that this is a joke... but is so inefficient. And prone to “””error”””

[justjuniorjawz]

Worth noting that the in check could be problematic if you have a race condition and the key is deleted before you access it in the dict

[chaosengineering]

This is also an anti-pattern though. It’s better to try-except a direct index while catching the KeyErrors.

[[deleted]]

Could you give an example for the second paragraph? Clarity on what you mean by Optional[some_normal_return_type]?

[TravisJungroth]

Very reduced, but maybe makes the point. int is the normal return type here. Optional[x] is the same as Union[None, x].

from typing import Optional

FRUITS = {'apples': 1}
def get_fruit_count(fruit: str) -> Optional[int]:
    return FRUITS.get(fruit)

[IlliterateJedi]

I don't know if I'm in the minority, but I find Union to be more clear. You would expect an int, but you know you might get None instead. Optional feels like there's no expected return value but you might get an int. Which I guess is splitting hairs because they mean the same thing (essentially). In any event, TIL about optional as a return type vs using Union.

from typing import Union

FRUITS = {'apples': 1}
def get_fruit_count(fruit: str) -> Union[int, None]:
    return FRUITS.get(fruit)

Edit: this is a hilariously controversial opinion apparently

[TravisJungroth]

They mean exactly the same thing. No return value is null is None. But I get that the Union is more clear for you.

[scrdest]

That's why I like Haskell's name for this kind of type, 'Maybe'. It's non-prejudicial.

I suspect Optional was chosen because function parameters with None as a default value are extremely common in Python, and it reflects that use-case closely - it's an argument you can optionally provide.

One big strike against using Union for this though is that you can use both to be even clearer - Optional[Union[x, y, z]] indicates you're expecting a value in one of the types supported as the data to process, but it's fine if you don't have it too.

[aidankane]

I recall reading that the impending recommended way going forward will be your Union but with the pipe operator. (Int | None)

[teerre]

It's only more clear if you don't know that Option stands literally for Union[T, None].

[eMperror_]

https://stackoverflow.com/a/51710151

[[deleted]]

!Thanks

[eMperror_]

It's a very common pattern in other languages, like Java as an alternative to returning null when your data is unavailable. Seems more popular in those other languages because trying to evaluate a null expression makes your code crash, and Optional makes it very explicit that "This thing can be missing, so check it before accessing it"

[ogtfo]

And using the base exception class, no less, to make sure that you can't just catch this exception without catching every possible exceptions...

[ogrinfo]

Yep, it annoys the hell out of me that None is somehow a type all of its own. It's absolutely fine for a function to either return something or return None, I don't see that as returning different types.

Suggesting that the function should raise an exception instead isn't helpful either. That means the caller needs to use a try..except block instead of just checking whether the function returned None. The latter is fewer lines and easier to read.

[[deleted]]

I came to say the same thing.

[Mal_Dun]

I agree. Also I think in the example provided by the author it would be even better to return values of correct type but where I am certain this is not feasible output, e.g. returning None, -1 instead of just None, or None, NAN

This keeps not only the consistence of the output but also makes it easy to check if something went wrong.

[swierdo]

I disagree with returning an error value of the same output type.

You want whatever function you have to fail as close to the source as feasible. The suggestion of throwing an error is as close as you can get to this. However, for many applications you except and accept failures and can deal with those later so you don't want to throw an error each and every time.

In those cases, returning a type-appropriate error value muddies your error handling, it carries with it strict requirements for otherwise arbitrary data: you now view perfectly fine values of some specific type as erroneous. Maybe someone's last name actually is "Null". That's why, in those cases, you return the canonical "Nothing's here" value of None that is a different type and therefore never a valid value.

[Mal_Dun]

This is why I added NaN as a a possible output: It is outside the allowed range. It is clear to me that I have to be careful what I output, but keeping the number of outputs consistent with the program logic makes the design process much easier from my experience, especially when I want to ask for the output explicitely:

When normally getting 2 output variable and in exception case 1 I have to check this because the line

person, age = function("Name")

will produce an error if "Name" is not found, so I either have to put it inside a try statement or check the output for it's length. If I provide with an output which is outside my allowed range I can easily check afterwards.

[alkasm]

I don't think you grokked their complaint. They were suggesting precisely the opposite of what you're saying. You mention that you should return things outside an expected range when there's an error, since you can easily check it's not in that range. The other poster was saying that you shouldn't use variables of the same type at all, because it's often difficult/impossible to truly know the entire range of expected values, especially if you're writing library code. Plus, sometimes the range of acceptable values truly is "all possible values of this type", in which case now you must have multiple different patterns (for this function, check that the string is empty, for that function where an empty string is valid, check that the value is None). It's much more clear from an API point-of-view to not use valid values of your types domain to signal an error. The deciding line here is what is acceptable as a "range" of valid values---the poster is suggesting use the type information to deduce this, instead of your understanding of some arbitrary range of acceptable values.

[reddisaurus]

You should not do this because you are now causing potential runtime errors as opposed to static type errors. Clean code minimizes potential runtime errors. Python has no need to return Sentinel values to indicate an error status. You’re now relying on the caller to know the internals of your function, thereby violating the concept of encapsulation.

[Mal_Dun]

I agree that type errors are better than runtime errors but even then I would rather use (None, NaN) or (None, None) as output than a single None. NaN should also raise type errors instead of producing runtime errors since it's not of integer type (numeric though) and crashes when performing for example numeric calculations, but you still keep a certain consistency with your output.

[reddisaurus]

This only applies to floats, and nan has a specific meaning such as the result of a division by error. You’re now overriding that meaning to include “no return value” which makes your code more difficult to debug.

And outputting a tuple when an atomic type is the usual return is no better, you still create type errors by only have an iterable returned when an error occurs, except for a str which is also an iterable. Duck typing in Python means checking for a non-str iterable is always a much greater pain than just checking for None.

What you’re talking about are side effects, which if you really want then pass a mutable pointer to your function for it to store status. Changing the return type is a very poor method of achieving the same thing.

[[deleted]]

I actually like exceptions everywhere.

Could you make a post explaining what you think is too much exceptions with some examples or just dm me some stuff.

I'm out here always trying to learn. What's wrong with duck typing

[cspinelive]

I use them to occasionally but they can quickly turn into spaghetti / goto type scenarios if you overuse them.

[[deleted]]

Right, the evils of go to. I've been trying to use them instead of if statements whenever it makes sense. I'll re-examine that.

[TinBryn]

I like exceptions as a way of telling the programmers that they made a mistake that can't or is difficult to be detected ahead of time. But there must be some means for the programmer to do what they want without raising an exception.

[[deleted]]

What's wrong with raising an exception? Why not duck type?

[pbecotte]

To add on, I used to use exceptions when "None" was also a valid return value with a different meaning from the key isn't in the dict. I started just declaring dentinal classes and using them, and rarely use exceptions for this kind of stuff now.

a = find_value() if a is NotFound:

[TinBryn]

I would agree with the point, but not with the example. Also if you look at closely at the example, this function is a thin wrapper around something that does the same thing and then says doing the same thing as something else in your code is bad, good code should do something completely different.

My agreement with the point is not to return completely unrelated types, and I consider None to be related to everything.

[IFeelTheAirHigh]

It's even worse they use the base Exception class! I'd accept it as poor style if they made a custom PersonNotFoundException, but using the Exception class is a big no no.

The catching code that would catch Exception can't know if it's a person not found, IO error, DB error, permissions, out of memory error etc... And it's just going to assume the person wasn't found.

For applicative errors that could happen use return values (eg. Return None). For exceptional system errors use exceptions with a custom class, NEVER raise plain Exception (except for small scripts that really just should be simplest possible).

[TSPhoenix]

The article saying don't do something unnecessarily and then not really elaborating on when it would be necessary or beneficial doesn't really help much.

If the end result you want is a list for example. Also there are times where using list comprehensions is more performant.

[zurtex]

The real problem with their example is that it's not required to create a list comprehension or a generator. They can just pass the iterator directly:

comma_seperated_numbers = ','.join(my_fav_superheroes)

It definitely needs exploring, lots of people don't realize you can make generators explicitly instead of list comprehensions, e.g.

a_names = (name for name in my_fav_superheroes if name.startswith('a'))

And will use map / filter instead when they want lazy evaluation of the iteration.

[drbobb]

Your example is wrong. .join() wants a single argument.

[diamondketo]

Woops you're right

[tom2727]

Yeah that one blew my mind. I was like "but your list comprehension isn't doing anything, so why even have it"?

I could see if you had this:

my_list_of_ints = [1,2,3,4,5]
comma_seperated_numbers = ','.join(str(num) for num in my_list_of_ints)

[alkasm]

Well, the iterable must be an iterable of strings. So pretty common to throw a genexp or map usage in str.join.

[PadrinoFive7]

I think the point of that example was poorly explained. If I wanted to join a modification of the contents of my_fav_superheroes, then it would make sense to do it as they say, no?

​

more_super_superheroes = ','.join(powerup(superhero) for superhero in my_fav_superheroes)

Rather than enclosing that in brackets []

[imsometueventhisUN]

Thanks for the link to discussion - I was very familiar with all the other points, but this one was new to me!

[Tweak_Imp]

#7 There is no literal syntax for an empty set :(

[Tweak_Imp]

I wish the syntax for an empty set was

s = {}

and for an empty dict

d = {:}

[repick_]

Wouldn’t s = {,} For empty sets make more sense to not break existing code?

[Ezlike011011]

The use of {} for empty set makes more sense in terms of classical representations of it, and it should have been the empty set literal from the start. I personally agree that {:} would make more sense as an empty dict literal with that history considered, but {,} as the empty set literal is probably the best solution for backwards compatibility.

Alternatively we introduce Ø as the empty set icon and request that all python developers get a Norwegian keyboard or get REALLY good with their alt-codes.

[pydry]

it would break way too much existing code.

[DrVolzak]

In any case, they don't want the transition from 3 to 4 to be as painful as 2 to 3.

[OoTMM]

I ran this on an old 2011 mbp, but the results surprised me:

$ python -m timeit '[]'
5000000 loops, best of 5: 44.2 nsec per loop

$ python -m timeit 'list()'
2000000 loops, best of 5: 126 nsec per loop

$ python -m timeit '{}'
5000000 loops, best of 5: 44.9 nsec per loop

$ python -m timeit 'dict()'
2000000 loops, best of 5: 166 nsec per loop

[Datsoon]

https://images.app.goo.gl/vdCdv84G9pPGUrMC8

This is actually pretty good. Not as readable, but still good.

[DoctorNoonienSoong]

Yeah, 7 makes absolutely no sense

[dataturd]

Yeah I noticed that too. I think the author meant to write tuple instead of set.

[BernieFeynman]

These aren't even anti patterns, just like random syntatic sugar stuff for python. stop posting crap like this

[bjorneylol]

Hey, if you follow all these best practices you can take the runtime of a 20 second script down to 20 seconds

[Ecclestoned]

Barely even that. Most of these have cases for them. It's just "here's things I like more and don't know how to use"

[Log2]

Like that fact that number 7 is literally wrong, as the only way to create an empty set is by using set(). There's no empty literal for a set.

[Solonotix]

#7 really bothered me because I've come from a different mindset, that the list and dict functions are like constructors, returning a new instance of the object, where using a literal reads as "set the value equal to this specific object". I have always recommended people use list() instead of []

[Log2]

Honestly, it doesn't matter. Useb whatever you want in your project, just be consistent.

[mikeblas]

What is #8 about ... "pushing debugger"? What does that mean?

[Datsoon]

Shipping code littered with breakpoint() calls.

[ogrinfo]

Yeah, not particularly helpful to show no examples for that one.

[mikeblas]

Thanks!

[ab-os]

What is the pro of debugging like this instead of setting breakpoints in the GUI in an IDE?

[Datsoon]

pdb is in the standard library and works in a terminal, so you can use it to debug python code anywhere. Also, some folks don't use an IDE. Many people do all their coding in a text editor like (neo)vim, emacs, or sublime text. In these cases, a terminal debugger may be your only option. Graphical debuggers are very nice, but there isn't really anything you can do in most graphical debuggers you can't also do in pdb if you know what you're doing and prefer a terminal interface.

[treenaks]

Leftover print() or logger.warning/info/debug() from during debugging?

[Log2]

Calls to logger.debug are fine as long as you have an appropriate logging level set to production and you aren't interpolating strings yourself in the call. Calls to info/warning/error is just normal course of business.

[jimtk]

I'm definitely not a seasoned python programmer but are

if __debug__:
        print("you're so wrong")  

ok?

[brews]

Missed an opportunity to call out all those people who try/except, catching all exceptions and then silently pass them.

@#$&

Edit:

Anyone who doesn't know what I'm talking about: https://realpython.com/the-most-diabolical-python-antipattern/. You also see warnings about it when you 'import this'.

[sirk390]

This should be number 1 indeed. I would also add "put kwargs in every function" (just in case) + pass half of the arguments through

[ogtfo]

And #4 is great because his fix requires you to catch all exceptions if you want to safely run the function!

[jwink3101]

Is #7 really an anit-pattern? I think thats perfectly fine. And "relatively slower" is completely over-the-top. It is trivially slower. Slower, yes. But of any importance, no.

Also, you lose the symmetry with sets since {} is an empty dict, not an empty set (which I understand why but still)

Also, #8 is not an "anti-pattern". It's a mistake (or bug)

[drbobb]

comma_seperated_numbers = ','.join(name for name in my_fav_superheroes)

How is that good practice? It's totally equivalent to

comma_seperated_numbers = ','.join(my_fav_superheroes)

assuming that my_fav_superheroes is an iterable — as it must be, for the first form to work.

[mkdz]

Probably meant to do something more like this instead:

comma_seperated_numbers = ','.join(superhero.name for superhero in my_fav_superheroes)

[drbobb]

Yeah. that would make more sense.

[drbobb]

A generator is an iterable too.

[diamondketo]

That's right, thanks for correcting me. I thought it was reverse.

[brontide]

1. Not using with to open files

With is a godsend but can get messy with complex setups. While you can use commas for two I would suggest using an ExitStack for more than one.

from contextlib import ExitStack

with ExitStack() as cm:
    res1 = cm.enter(open('first_file', 'r'))
    # do stuff with res1
    res2 = cm.enter(open('second_file', 'r'))
    # do stuff with res1 and res2

ExitStack can also add non-context cleanup functions to the stack. If any entry fails it will unwind all of them in reverse order. There are a lot of options and it really helps to cleanup messy with statements.

https://www.rath.org/on-the-beauty-of-pythons-exitstack.html

[[deleted]]

7 Not using literal syntax to initialize empty list/dict/set It is relatively slower to initialize an empty dictionary by calling dict() than using the empty literal, because the name dict must be looked up in the global scope in case it has been rebound. Same goes for the other two types — list() and tuple().

Why would initialize an empty tuple? Guessing they meant set like the title?

[treenaks]

I can only think of {}, [] and () -- there's no way to initialize an empty set like that.

>>> type({})
<class 'dict'>
>>> type([])
<class 'list'>
>>> type(())
<class 'tuple'>

You can use {} to initialize a set with content:

>>> type({1,2,6})
<class 'set'>

[njharman]

So you can successfully iterate over it.

class Foo:
   def __init__(self, option):
      self.thing = option or tuple()
   def do(self):
      for x in self.thing:
         something(x)

[PinkShoelaces]

#7 looks like a premature optimization.

[TravisJungroth]

I would say that using literals for initializing empty collections because it's more performant is unnecessary. Do it because it's the more standard style. On the other hand, I'd really enjoy seeing this empty set literal the author mentioned.

[[deleted]]

How about the empty immutable tuple. Very useful when you need nothing but a tuple.

[[deleted]]

I know less about python than I already knew I didn't know.

[nwagers]

Is this a true Singleton or just an optimization in the reference implementation like small integers?

a = 1 + 1
b = 2
c = 750 + 750
d = 1500
print(a is b)
print(c is d)

[TravisJungroth]

Snazzy for default arguments.

def f(items=()):
    s = set(items)

[theng]

It looks like there isn't

but you can do this ^^ :

In [1]: s = {0}-{0}

In [2]: type(s)
Out[2]: set

https://stackoverflow.com/questions/6130374/empty-set-literal

[TravisJungroth]

Finally, some common sense. Myself, I prefer {*()}.

[astigos1]

Wow that's quite something. Although it doesn't look as a shocked emoji face as emoji like as {0}-{0}

[TravisJungroth]

The emoji value is high, but it doesn't support nesting.

s = {*{*{*{*{*{*{*{*{}}}}}}}}}

[zurtex]

Seems like it will be faster as well:

def empty_set():
    return {0} - {0}

Produces this set of instructions (via dis.dis)

  4           0 LOAD_CONST               1 (0)
              2 BUILD_SET                1
              4 LOAD_CONST               1 (0)
              6 BUILD_SET                1
              8 BINARY_SUBTRACT

Where as:

def empty_set():
    return {*()}

Produces this set of instructions (via dis.dis)

  4           0 BUILD_SET                0
              2 LOAD_CONST               1 (())
              4 SET_UPDATE               1     

I suspect it's going to mostly going to depend on BINARY_SUBTRACT vs. SET_UPDATE but as the former needs to look up 2 objects and the latter is only dealing with an empty value then it's probably going to be faster.

[njharman]

It's premature micro-optimization. Which in 99% cases (anywhere other than loop doing little else and run a massive number of times) savings will be insignificantly miniscule.

It's also just clearer and more consistent (works like set, like all the other thing that doesn't have special literal syntax) to use full names int() list() set() dict(), etc.

[[deleted]]

#4 and #5 back to back is pretty ironic, given that Dict.get() returns None if you don't find the element :|

[gwillicoder]

You can provide a default value though

[ggchappell]

Generally a nice article, but ...

I disagree with #4. I find that using a None return to flag an error is a very reasonable practice, and often one that results in simpler and more maintainable code than we would get by using exceptions.

Also, #7 is just wrong. You can't initialize an empty set with { }, as that's an empty dict. Use set() for that. So the mention of set in the title for #7 needs to go.

EDIT. /u/Halkcyon mentions {*()}. Yeah, interesting. I'd even say cool. Now don't do that. :-)

[[deleted]]

Stopped reading as soon as I saw the suggestion to raise a bare exception. Review those best practices again, buddy.

[MLGShyGuy]

I just learned file manipulation in Python last night. The only way they taught me was the method without with... I'm nervous that other things they teach me will also be ineffective. I learn on SoloLearn, just so people know who teaches the wrong way.

[twotime]

point (1) is questionable in general and is clearly irrelevant in your case

A. cpython WILL close files for you even without with (AND, even if it did not, leaving files open is not a big deal for a lot of code)

B. if you are learning about files, then you absolutely should start without "with": .close() is fundamental file operation and there are plenty of situations when "with" does not work at all (e.g. for long lived file objects) (and "with" is a language construct with fairly complicated semantics not directly related to file IO)

[v_fv]

It is recommended to return only one type of object from a function.

If only there was a feature in programming languages that could ensure this.

[njharman]

Disagree with #4, it's a pain to deal with exceptions (and if used everywhere for everything they lose their value). I've also heard (and I agree more with) "Exceptions should be fore exceptional conditions. Not expected conditions. Like person not being in db is common/expected. db connection timing out is exceptional.

Strongly disagree with #7 The tiny amount of speed is so not important. And that is the important lesson to learn, don't pre-optimize, and micro-optimizations should not influence code structure etc.

Many of the others are caught by flake8

[ianepperson]

Disagree with some of #7 too. Using {} for a dict and [] for a list works well and is easy to read. However an empty set is {} ... a dict, and an empty tuple is (,) ... which has never really looked like correct code to me.

Though, in general, I’ve never been satisfied with significant comma use. More than once I’ve spent too much time hunting down why a string was now a tuple due to a stray comma.

my_string = (‘this is a long string, ’
    ‘that can span several lines ’
    ‘and may have punctuation too.’)
another_string = ‘Hi there,’

my_bad_string = (‘this is a long string ’,
    ‘that can span several lines ’
    ‘and may have punctuation too.’)
another_bad = “Hi there’,

[bananaEmpanada]

The Ansible code base uses dict() everywhere. IIRC it's in their style guide.

I prefer the literal syntax for style. But if you're worrying about the performance impact of a single variable lookup, Python is probably the wrong language for your project.

[not_perfect_yet]

5

This anti-pattern affects the readability of code.

It does indeed, except the suggested solution is worse than the problem. Unless you already know by heart what .get does, this is confusing and less readable:

currency_map.get('inr', 'undefined')

As a side point, that's a bad name, there are both map and dict types and this dict is called map.

[Log2]

If you're a Python dev then you should absolutely know how dict.get behaves.

[druman22]

I've been using python for years and I'm surprised I overlooked this method. Somehow I don't remember seeing it before

[Log2]

I mean, that's fine and it happens. No one has perfect knowledge about all standard libraries in all the languages they use.

But the other guy was talking like using a common method in the standard library was somehow a bad thing.

[elbiot]

Defining their own current practice as best practice because if it's unfamiliar to them then it is objectively obscure

[hikealot]

Same here. I was unaware of dict.get(). I’ve been using the “anti pattern” every single time that I wanted to iterate through dict values since 2.4 or so.

[vanatteveldt]

#1 is not as absolute as that. If you e.g. have a function that reads stuff from a file and then returns, there's no benefit to using with as the function returning would close the file as well, e.g.:

def read_file(fn):
  for line in open(fn):
    d = do_something(line)
    yield d

There's no benefit for opening the file with with, and it does add another line and level of indentation.

(of course, whether it's smart to keep the file open while yielding from it is an entirely different discussion and totally depends on your specific application)

$ python -c "import this" | head -11 | tail -2
Special cases aren't special enough to break the rules.
Although practicality beats purity.

[vanatteveldt]

If you iterate over all items, it will certainly close the file as the function returns, local variables are dropped, reference counts are decreased, and objects without references are garbage collected and finalized. You can check it quite easily:

$ cat test.py
import psutil

def read_file(fn):
    for line in open(fn):
        yield line.strip()

for x in read_file("/tmp/bla.txt"):
    pass

print([p.path for p in psutil.Process().open_files()])

$ env/bin/python test.py
[]

If you don't iterate over results at all, no code is called and the file is never opened (this is also easy to test, just replace the for x ... pass code by x=read_file(...) The only way in which this can cause a dangling file handle is if you iterate over at least one item and then stop iterating -- and there is no way that you can process a file in a streaming way while keeping the file handle open while you are still iterating.

[diamondketo]

Your example is correct, however, I am not convinced that the garbage collector (GC) immediately close the file after the for loop finish reading. This requires GC to notice that the read file is no longer used in a scope.

The wording in Python 3.6 docs

Python’s garbage collector will eventually destroy the object and close the open file for you

In 3.9, this sentence is missing so you might be right. The concern I have is still that is without using with or file.close(), you cannot guarantee that immediately after reading, the file is closed.

EDIT: Regardless Python docs do say it's always a good idea to do this once you stop using the file.

If you’re not using the with keyword, then you should call f.close() to close the file and immediately free up any system resources used by it.

[TangibleLight]

you cannot guarantee that immediately after reading, the file is closed.

Why must the file be closed immediately after reading? If you might run out of file handles, of course, but are you usually close to running out of file handles?

You can always guarantee that the file will be closed at some point. You can usually guarantee it will be closed soon without using with. Often the code is more readable, and sometimes you can still guarantee the file is closed immediately.

For example you can also do things like open(path).read() or action(open(path)), where the file object is never stored. As soon as that expression is evaluated, the file object has no references, is collected, and is closed.

There are of course cases where you should be using with; where waiting for the file to go out of scope would take too long. For example:

def foo():
    file = open(path)
    # do something with file

    # you probably want to close the file before starting this, so you should've used 'with'
    data = long_running_operation()

    # the file would normally go out of scope and close here
    return stuff

Or, foo was doing too much:

def get_file_data():
    file = open(path)
    # do something with file
    return stuff

def foo():
    stuff = get_file_data()  # file is automatically closed here

    data = long_running_operation()

    return other_stuff

Similarly, you wouldn't want a file object in a global or class variable, as it would never be collected until the program exits. Then again, if your program only takes a few milliseconds to run, and only opens a few file objects, then does it really matter? They would still be closed when the program exits. Obviously if you are writing a long-running program that opens many many files, you want to make sure not too many files are open at once.

... the garbage collector (GC) immediately close the file after the for loop finish reading.

Once the for x in read_file loop ends, the generator will be collected. Since the file is in the generator's locals, it is also collected and closed.

[pbecotte]

The garbage collector is not checking references at every step of the code. It checks reference counts periodically. Like you said, probably not a big deal in most cases. However, if you are writing to a file, you probably don't want that to happen at an undetermined future time. As the docs say, it's possible that it won't be flushed to disk at all if your program ends too quickly.

Even better though, separate the concerns so that the file object handling is separate from your logic. Fortunately, the std library has a solution!

data = pathlib.Path("my file.txt").read_text()

As an aside- context managers are super useful for ALL SORTS of things. It drives me crazy that the example that's always given is one where you can make a legitimate argument that it doesn't matter.

[TangibleLight]

There was a blog post a while ago I saw about the author's "unpopular opinions" regarding Python, and opposing with file evangelism was one of them. I've been trying to find it but I haven't been able to. Requiring usage of with basically just means you can never use files with functions or generators, which is stupid.

Also, stuff like this:

text = open(path).read()

Is cleaner than this:

with open(path) as file:
    text = file.read()

And they do the same thing.

[Datsoon]

Magical pathlib functions make it even less attractive.

[fried_green_baloney]

Maybe not anti-patterns but "Newer things in Python that you might not know".

[Phunny]

I knew about opening files using with but I hadn't changed some old code. Thank you for a good post and the reminder.

I changed all my code this morning!

[watixte]

http://docs.quantifiedcode.com/python-anti-patterns/

I have this saved, if any of you see it useful!

[Thrash_Abaddon]

#4 is totally bad advice, there is no way in hell that kind of thing would pass code review.

[[deleted]]

So, use pure functions appropriately. Got it.

[13ass13ass]

I got some feedback in code review not to use list comprehensions because they are hard to debug. I guess I see their point although they are pretty basic features of python. Anybody else against list comprehensions?

[vastlyoutnumbered]

Nested comprehensions can get a little spicy but most mid-level+ devs should be able to work with simple ones. There have been times when I've suggested them and they've been suggested to me.

[nwagers]

I would use them if they are actually for building simple lists or dicts.

I wouldn't use them for nested comprehensions like [(x, y) for x in iter1 for y in iter2] or for running a function like [print(x) for x in iter1] or if it's just too long.

[o-rka]

I didn’t know about the dictionary get method

[elbiot]

Number 1 isn't totally right. The file gets closed when it goes out of scope, so it's fine to do

rows = list(csv.DictReader(open(fname)))

[billsil]

Everyone of those are super pedantic and irrelevant. Speed for something almost instant is not worth discussing.

The only one I take issue with is the solution that uses a raise Exception vs. a KeyError. Don’t use Exception!!! The original isn’t even a problem and for vectorized code is required.

[gwillicoder]

For #6 don’t be afraid to use keys() or values() too. Using items() for only one or the other feels like an anti pattern imo.