Is this unpythonic?

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Steven D'Aprano-11
On Fri, 8 May 2015 06:01 pm, Frank Millman wrote:

> Hi all
>
> I have often read about the gotcha regarding 'mutable default arguments'
> that frequently trips people up.
>
> I use them from time to time, but I have never had a problem. I have just
> delved a bit deeper to see if I am skating on thin ice.
>
> AFAICT my usage is safe. If I use a list as an argument, I only use it to
> pass values *into* the function, but I never modify the list. So if I omit
> the argument, the original default empty list is used, otherwise the list
> that I pass in is used.
>
> However, every time I look at my own code, and I see   "def x(y, z=[]):
> ....."   it looks wrong because I have been conditioned to think of it as
> a gotcha.

It is a gotcha, and a code smell.

http://www.joelonsoftware.com/articles/Wrong.html

You can use it, but with care: code smells aren't necessarily wrong, they
just need to be looked at a little more carefully.


> Would it be more pythonic to change them all to use the alternative
> "z=None", or is it ok to leave it as it is? Or to phrase it differently,
> how would an experienced pythonista react on seeing this when reviewing my
> code?

I would change it to z=None *unless* you actually required the list to be
mutated, e.g. if you were using it as a cache.

Does z have to be a list? Could you use an empty tuple instead?

def x(y, z=()): ...



--
Steven



Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Frank Millman

"Steven D'Aprano" <steve+comp.lang.python at pearwood.info> wrote in message
news:554c8b0a$0$12992$c3e8da3$5496439d at news.astraweb.com...
> On Fri, 8 May 2015 06:01 pm, Frank Millman wrote:
>
>> Hi all
>>
[...]
>>
>> However, every time I look at my own code, and I see   "def x(y, z=[]):
>> ....."   it looks wrong because I have been conditioned to think of it as
>> a gotcha.
>
> It is a gotcha, and a code smell.
>
> http://www.joelonsoftware.com/articles/Wrong.html
>

Interesting read - thanks.

> You can use it, but with care: code smells aren't necessarily wrong, they
> just need to be looked at a little more carefully.
>
>
>> Would it be more pythonic to change them all to use the alternative
>> "z=None", or is it ok to leave it as it is? Or to phrase it differently,
>> how would an experienced pythonista react on seeing this when reviewing
>> my
>> code?
>
> I would change it to z=None *unless* you actually required the list to be
> mutated, e.g. if you were using it as a cache.
>

Ok, you and Joel have convinced me. I will change it to z=None.

> Does z have to be a list? Could you use an empty tuple instead?
>
> def x(y, z=()): ...
>

That was Chris' suggestion as well (thanks Chris).

The idea appealed to me, but then I found a situation where I pass in a
dictionary instead of a list, so that would not work.

Replacing them all with None is cleaner and, I now agree, more pythonic.

Thanks for the good advice.

Frank





Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Dave Angel-4
On 05/08/2015 06:53 AM, Frank Millman wrote:

>
> "Steven D'Aprano" <steve+comp.lang.python at pearwood.info> wrote in message
> news:554c8b0a$0$12992$c3e8da3$5496439d at news.astraweb.com...
>> On Fri, 8 May 2015 06:01 pm, Frank Millman wrote:
>>
>>> Hi all
>>>
> [...]
>>>
>>> However, every time I look at my own code, and I see   "def x(y, z=[]):
>>> ....."   it looks wrong because I have been conditioned to think of it as
>>> a gotcha.
>>

It might be appropriate to define the list at top-level, as

EMPTY_LIST=[]

and in your default argument as
     def x(y, z=EMPTY_LIST):

and with the all-caps, you're thereby promising that nobody will modify
that list.

(I'd tend to do the None trick, but I think this alternative would be
acceptable)

--
DaveA


Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Frank Millman

"Dave Angel" <davea at davea.name> wrote in message
news:554CA652.1000607 at davea.name...

> On 05/08/2015 06:53 AM, Frank Millman wrote:
>>
>
> It might be appropriate to define the list at top-level, as
>
> EMPTY_LIST=[]
>
> and in your default argument as
>     def x(y, z=EMPTY_LIST):
>
> and with the all-caps, you're thereby promising that nobody will modify
> that list.
>
> (I'd tend to do the None trick, but I think this alternative would be
> acceptable)
>

Thanks, Dave, I like that idea.

However, as you can see from my other replies, I have decided to go with the
flow, and use the None trick.

Frank





Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Steven D'Aprano-11
In reply to this post by Steven D'Aprano-11
On Fri, 8 May 2015 08:53 pm, Frank Millman wrote:

>> Does z have to be a list? Could you use an empty tuple instead?
>>
>> def x(y, z=()): ...
>>
>
> That was Chris' suggestion as well (thanks Chris).
>
> The idea appealed to me, but then I found a situation where I pass in a
> dictionary instead of a list, so that would not work.


Why wouldn't it work? If it worked with an empty list, it will probably work
with an empty tuple instead.



--
Steven



Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Frank Millman

"Steven D'Aprano" <steve+comp.lang.python at pearwood.info> wrote in message
news:554cd119$0$12977$c3e8da3$5496439d at news.astraweb.com...

> On Fri, 8 May 2015 08:53 pm, Frank Millman wrote:
>
>>> Does z have to be a list? Could you use an empty tuple instead?
>>>
>>> def x(y, z=()): ...
>>>
>>
>> That was Chris' suggestion as well (thanks Chris).
>>
>> The idea appealed to me, but then I found a situation where I pass in a
>> dictionary instead of a list, so that would not work.
>
>
> Why wouldn't it work? If it worked with an empty list, it will probably
> work
> with an empty tuple instead.
>

Sorry, I should have been more explicit. In the case of a dictionary, I used
'def x(y, z={}'

I have not checked, but I assume that as dictionaries are mutable, this
suffers from the same drawback as a default list.

Unlike a list, it cannot be replaced by an empty tuple without changing the
body of the function.

Dave's suggestion would have worked here -

    EMPTY_LIST = []
    EMPTY_DICT = {}

But as I have decided to use the None trick, I use it for a default
dictionary as well.

Frank







Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Frank Millman

"Frank Millman" <frank at chagford.com> wrote in message
news:mik7j6$59n$1 at ger.gmane.org...

>
> "Steven D'Aprano" <steve+comp.lang.python at pearwood.info> wrote in message
> news:554cd119$0$12977$c3e8da3$5496439d at news.astraweb.com...
>> On Fri, 8 May 2015 08:53 pm, Frank Millman wrote:
>>
>>>> Does z have to be a list? Could you use an empty tuple instead?
>>>>
>>>> def x(y, z=()): ...
>>>>
>>>
>>> That was Chris' suggestion as well (thanks Chris).
>>>
>>> The idea appealed to me, but then I found a situation where I pass in a
>>> dictionary instead of a list, so that would not work.
>>
>>
>> Why wouldn't it work? If it worked with an empty list, it will probably
>> work
>> with an empty tuple instead.
>>
>
> Sorry, I should have been more explicit. In the case of a dictionary, I
> used 'def x(y, z={}'
>
> I have not checked, but I assume that as dictionaries are mutable, this
> suffers from the same drawback as a default list.
>
> Unlike a list, it cannot be replaced by an empty tuple without changing
> the body of the function.
>
> Dave's suggestion would have worked here -
>
>    EMPTY_LIST = []
>    EMPTY_DICT = {}
>
> But as I have decided to use the None trick, I use it for a default
> dictionary as well.
>

Cough, cough, I really should have given that a moment's thought before
posting.

It just dawned on me that a dictionary *can* be replaced by an empty tuple
without changing the body of the function.

There are two operations I might perform on the dictionary -

1. iterate over the keys and retrieve the values

2: use 'in' to test if a given string exists as a key

Both of these operations will work on a tuple and give the desired result,
so it is a very valid workaround.

More testing needed ...

Frank





Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Gregory Ewing
In reply to this post by Frank Millman
Frank Millman wrote:
> There are two operations I might perform on the dictionary -
>
> 1. iterate over the keys and retrieve the values
>
> 2: use 'in' to test if a given string exists as a key
>
> Both of these operations will work on a tuple and give the desired result,
> so it is a very valid workaround.

Although if I were reviewing a function like that,
it would probably make me pause for a moment or two
to consider why a tuple was being used as a value
for a parameter that is ostensibly supposed to be
a dict, and convince myself that it was okay.

The absolutely clearest way to write it would
probably be

    def f(things = None):
       "things is a mapping of stuff to be operated on"
       if things:
          for key in things:
             value = things[key]
             ...

A default value of None is a well-established idiom
for "this parameter is optional", and "if x:" is
idiomatic for "if I've been given an x", so writing it
that way has the best chance of passing the "pretty much
what you expect" test for good code. :-)

--
Greg


Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Frank Millman

"Gregory Ewing" <greg.ewing at canterbury.ac.nz> wrote in message
news:cr5sc6FgfmiU1 at mid.individual.net...

> Frank Millman wrote:
>
> The absolutely clearest way to write it would
> probably be
>
>    def f(things = None):
>       "things is a mapping of stuff to be operated on"
>       if things:
>          for key in things:
>             value = things[key]
>             ...
>
> A default value of None is a well-established idiom
> for "this parameter is optional", and "if x:" is
> idiomatic for "if I've been given an x", so writing it
> that way has the best chance of passing the "pretty much
> what you expect" test for good code. :-)
>

Thanks, Greg, that makes a lot of sense.

My original method of using 'z=[]' worked, but would cause a reviewer to
stop and think about it for a moment.

Changing it to 'z=()' would have pretty much the same effect.

Using 'z=None' would cause the least hesitation, and is therefore I think
the most pythonic.

It does require an additional test every time the function is called, but
the effect of that in my application is virtually zero.

If the overhead did become an issue, Dave's suggestion of 'z=EMPTY_LIST'
would be a good solution.

Frank





Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Johannes Bauer
In reply to this post by Frank Millman
On 08.05.2015 14:04, Dave Angel wrote:

> It might be appropriate to define the list at top-level, as
>
> EMPTY_LIST=[]
>
> and in your default argument as
>     def x(y, z=EMPTY_LIST):
>
> and with the all-caps, you're thereby promising that nobody will modify
> that list.
>
> (I'd tend to do the None trick, but I think this alternative would be
> acceptable)

I think it's a really bad idea to use a module-global mutable
"EMPTY_LIST". It's much too easy this happens:

# Globally
>>> EMPTY_LIST = [ ]

# At somewhere in the code at some point in time
>>> foo = EMPTY_LIST
>>> foo.append(123)
>>> print(foo)
[123]

# Some other place in code
>>> bar = EMPTY_LIST
>>> print(bar)
[123]

Regards,
Johannes

--
>> Wo hattest Du das Beben nochmal GENAU vorhergesagt?
> Zumindest nicht ?ffentlich!
Ah, der neueste und bis heute genialste Streich unsere gro?en
Kosmologen: Die Geheim-Vorhersage.
 - Karl Kaos ?ber R?diger Thomas in dsa <hidbv3$om2$1 at speranza.aioe.org>


Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Frank Millman

"Johannes Bauer" <dfnsonfsduifb at gmx.de> wrote in message
news:min3f0$2gh$1 at news.albasani.net...
On 08.05.2015 14:04, Dave Angel wrote:

> > It might be appropriate to define the list at top-level, as
> >
> > EMPTY_LIST=[]
> >
> > and in your default argument as
> >     def x(y, z=EMPTY_LIST):
> >
> > and with the all-caps, you're thereby promising that nobody will modify
> > that list.

> I think it's a really bad idea to use a module-global mutable
> "EMPTY_LIST". It's much too easy this happens:

> # Globally
> >>> EMPTY_LIST = [ ]

> # At somewhere in the code at some point in time
> >>> foo = EMPTY_LIST
> >>> foo.append(123)
> >>> print(foo)
> [123]

> # Some other place in code
> >>> bar = EMPTY_LIST
> >>> print(bar)
> [123]


A fair point. How about this as an alternative?

If one were to use this technique at all, it would be necessary to add a
comment at the top explaining the reason for this odd declaration.

It is then a simple extra step to say -

EMPTY_L:IST = ()

and if required -

EMPTY_DICT = ()

and expand the explanation to show why a tuple is used instead.

So if there was a situation where the overhead of testing for None became a
problem, this solution offers the following -

1. it solves the 'overhead' problem
2. it reads reasonably intuitively in the body of the program
3. it is safe
4. it should not be difficult to write a suitable self-explanatory comment

Frank





Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Johannes Bauer
In reply to this post by Johannes Bauer
On 10.05.2015 10:58, Frank Millman wrote:

> It is then a simple extra step to say -
>
> EMPTY_L:IST = ()
>
> and if required -
>
> EMPTY_DICT = ()
>
> and expand the explanation to show why a tuple is used instead.
>
> So if there was a situation where the overhead of testing for None became a
> problem, this solution offers the following -
>
> 1. it solves the 'overhead' problem
> 2. it reads reasonably intuitively in the body of the program
> 3. it is safe
> 4. it should not be difficult to write a suitable self-explanatory comment

I do understand what you're trying to do, but it is my gut-feeling that
you're overengineering this and as a side-effect introducing new problems.

With the above declaration as you describe, the code becomes weird:

foo = EMPTY_LIST
foo.append(123)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'tuple' object has no attribute 'append'

and

foo = EMPTY_DICT
foo["bar"] = "moo"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'tuple' object does not support item assignment

So instead, the user of this construct would have to

foo = list(EMPTY_LIST)

or

foo = dict(EMPTY_DICT)

but, coincidentially, this is easier (and more pythonic) by doing

foo = list()
foo = dict()

to which there are the obvious (pythonic) shortcuts

foo = [ ]
foo = { }

All in all, I'd be more confused why someone would introduct
"EMPTY_LIST" in the first place and think there's some strange weird
reason behind it. Explaining the reason in the comments doesn't really
help in my opinion.

Best regards,
Johannes

--
>> Wo hattest Du das Beben nochmal GENAU vorhergesagt?
> Zumindest nicht ?ffentlich!
Ah, der neueste und bis heute genialste Streich unsere gro?en
Kosmologen: Die Geheim-Vorhersage.
 - Karl Kaos ?ber R?diger Thomas in dsa <hidbv3$om2$1 at speranza.aioe.org>


Reply | Threaded
Open this post in threaded view
|

Is this unpythonic?

Frank Millman

"Johannes Bauer" <dfnsonfsduifb at gmx.de> wrote in message
news:min9t3$e56$1 at news.albasani.net...
On 10.05.2015 10:58, Frank Millman wrote:

> > It is then a simple extra step to say -
> >
> > EMPTY_L:IST = ()
> >
> > and if required -
> >
> > EMPTY_DICT = ()
> >
> > and expand the explanation to show why a tuple is used instead.
> >
> > So if there was a situation where the overhead of testing for None
> > became a
> > problem, this solution offers the following -
> >
> > 1. it solves the 'overhead' problem
> > 2. it reads reasonably intuitively in the body of the program
> > 3. it is safe
> > 4. it should not be difficult to write a suitable self-explanatory
> > comment

> I do understand what you're trying to do, but it is my gut-feeling that
> you're overengineering this and as a side-effect introducing new problems.

This has actually gone beyond a practical suggestion, and become more of an
academic exercise, so I don't think 'overengineering' comes into it. No-one
is recommending that this should be used in real-world code.

> With the above declaration as you describe, the code becomes weird:
>
> foo = EMPTY_LIST
> foo.append(123)
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> AttributeError: 'tuple' object has no attribute 'append'
>
> and
>
> foo = EMPTY_DICT
> foo["bar"] = "moo"
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> TypeError: 'tuple' object does not support item assignment

The whole point of this admittedly odd declaration is that no-one should do
anything with it at all. It is simply a place-holder to be used in the
absence of a real list or dict provided by the caller of the function. The
starting premise was that the function would only read from the list/dict,
not try to modify it.

Frank