2

This is not a problem I've encountered, but rather just a curiosity about best practices/style in python:

I see stuff like this quite often:

class A:
    def __init__(self, qux):
        """...do whatever... maybe this is expensive - maybe it isn't - maybe I don't know if it's expensive"""


def get_baz(foo, bar=A(2)):
    """...do another thing..."""
    return baz

I largely avoid function/method calls (like bar=A(2)) in function signatures, because this makes A.__init__ run on import. Do you think avoiding the running of code on import is best practice, or am I just OCD and it's totally fine to handle this on a case-by-case basis? I feel like there is an expectation (at least in the python community) that importing a package/module shouldn't take a significant amount of time/memory.

The common alternative to the above is slightly more verbose, but runs no extra code on import:

def get_baz(foo, bar=None):
    if bar is None:
        foo = A()

Which do you like more and why? Should I just not care?

(For context, I started thinking about this because importing an internal package at my company took 8 seconds, which was triggering.)

Battery_Al
  • 779
  • 1
  • 5
  • 20
  • 2
    `bar=A(2)` can make people *think* `A` is run every time `get_baz()` called with no `bar`. It doesn't, so it's misleading to readers. (It only evaluates `A(2)` _once_, and reuses that result as the default for all future calls of `get_baz()`). – Charles Duffy Jun 23 '20 at 16:07
  • 1
    Strictly speaking, I think your question likely falls foul of the "asking for opinion" SO anti-pattern, but for the record, I've never seen anything like this myself before, and would feel quite uncomfortable about seeing it in any code-base I looked after. That's an entirely subjective opinion however. I prefer function default parameters to be set to None, and then individually set after testing their value is None during setup time. – Thomas Kimber Jun 23 '20 at 16:07
  • ...so, the `bar=None` code is less surprising, insofar as folks are less likely to misunderstand its operation. (Indeed, insofar as you're describing the two as only a stylistic difference changing only whether "extra code is run on import", and not what code is invoked at runtime, it looks like the above distinction is surprising to you as well). – Charles Duffy Jun 23 '20 at 16:07
  • 1
    Also keep in mind that when you use a mutable object as a default parameter value, any modification to the object within the function would be reflected in subsequent calls to the function. This makes its behavior fundamentally different from using `None` as a default parameter value. – blhsing Jun 23 '20 at 16:12
  • 1
    @CharlesDuffy since `bar=A(2)` and `bar=None` result in different behavior, it's quite possible that only one is appropriate in the context of the overall code. – Mark Ransom Jun 23 '20 at 16:13
  • P.S. I definitely wouldn't call this an anti-pattern, just unnecessarily confusing. – Mark Ransom Jun 23 '20 at 16:14
  • 1
    Related: https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument – Barmar Jun 23 '20 at 16:19
  • 1
    @MarkRansom, even if `bar=A(2)` is what the OP actually wants, I'd be using a more explicit way to get it -- f/e, `BAR_DEFAULT = A(2)` at module level, and then `bar=BAR_DEFAULT`. Depending on behavior that those of us here know trips people up regularly (is a source of constant questions) is hard to defend. – Charles Duffy Jun 23 '20 at 16:24
  • 1
    `A.__init__` doesn't run on import, just `A(2)`. I think it would be normal to do `bar=decimal.Decimal('0')`. Plus, `bar=10000` is creating an `int` object and `bar=(1,2,3)` is creating a tuple. I think the criteria is whether it makes sense to call the function or create the class instance at import. If yes, then do it. – tdelaney Jun 23 '20 at 16:38
  • @blhsing - this is a great point which I overlooked (about mutability). With this I conclude `bar=A(2)` is *objectively* a bad idea. @CharlesDuffy - yeah I'm just realizing that people who write this definitely think that `A` is getting called with each function call. Another point against! @MarkRansom - tbh I used "anti-pattern" in the title as clickbait. But... considering the point about mutability, now I actually do think it's an anti-pattern! – Battery_Al Jun 23 '20 at 16:45
  • 2
    @Battery_Al Using a mutable object as a default parameter value is not necessarily a bad idea, but can be a preferred approach when the state of the object is meant to be carried over from call to call. It is commonly used to implement a cache for memoization, for example. See: https://stackoverflow.com/questions/55068876/recursion-memoization-and-mutable-default-arguments-in-python – blhsing Jun 23 '20 at 16:48
  • 1
    @Battery_Al I think the anti-pattern is to mutate a passed argument, whether it's default or not. Python makes it so easy to return multiple values from a function, so just return a mutated copy of the object and let the caller reassign it as necessary. – Mark Ransom Jun 23 '20 at 17:17
  • @blhsing - that's a cool use, but I do think making the cache usage more explicit is probably a good idea usually (i.e., having like a `_cache` instance attribute on a `Fibonacci` class), only because this might be confusing if the function were more complicated than a fibonacci calculator. – Battery_Al Jun 23 '20 at 18:05
  • @tdelaney - do you mean that `__new__` gets run on import, but not `__init__`? – Battery_Al Jun 23 '20 at 18:11
  • @Battery_Al - Oh, right.... I read that all wrong. Yes, `A.__init__` is called. I was focusing on `bar=A(2)`. The general comment I think is still right. This is fine as long as you don't mind calling `A.__init__` at import. Its not generally an antipattern (in fact many things like the `Decimal` example I gave are normally done this way) but is unwise on a case by case basis. – tdelaney Jun 23 '20 at 19:23

0 Answers0