Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #90123 > unrolled thread

Is this unpythonic?

Back to article view | Back to comp.lang.python

Contents

  Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-08 10:01 +0200
    Re: Is this unpythonic? Steven D'Aprano <steve+comp.lang.python@pearwood.info> - 2015-05-08 20:08 +1000
      Re: Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-08 12:53 +0200
        Re: Is this unpythonic? Steven D'Aprano <steve+comp.lang.python@pearwood.info> - 2015-05-09 01:07 +1000
          Re: Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-09 07:56 +0200
          Re: Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-09 08:51 +0200
            Re: Is this unpythonic? Gregory Ewing <greg.ewing@canterbury.ac.nz> - 2015-05-09 19:47 +1200
              Re: Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-09 11:10 +0200
      Re: Is this unpythonic? Dave Angel <davea@davea.name> - 2015-05-08 08:04 -0400
        Re: Is this unpythonic? Johannes Bauer <dfnsonfsduifb@gmx.de> - 2015-05-10 10:04 +0200
          Re: Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-10 10:58 +0200
            Re: Is this unpythonic? Johannes Bauer <dfnsonfsduifb@gmx.de> - 2015-05-10 11:54 +0200
              Re: Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-10 12:40 +0200
      Re: Is this unpythonic? "Frank Millman" <frank@chagford.com> - 2015-05-08 15:12 +0200

#90123 — Is this unpythonic?

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.

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?

Thanks

Frank Millman

[toc] | [next] | [standalone]

#90132

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

[toc] | [prev] | [next] | [standalone]

#90138

"Steven D'Aprano" <steve+comp.lang.python@pearwood.info> wrote in message 
news:554c8b0a$0$12992$c3e8da3$5496439d@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

[toc] | [prev] | [next] | [standalone]

#90172

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

[toc] | [prev] | [next] | [standalone]

#90211

"Steven D'Aprano" <steve+comp.lang.python@pearwood.info> wrote in message 
news:554cd119$0$12977$c3e8da3$5496439d@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

[toc] | [prev] | [next] | [standalone]

#90214

"Frank Millman" <frank@chagford.com> wrote in message 
news:mik7j6$59n$1@ger.gmane.org...
>
> "Steven D'Aprano" <steve+comp.lang.python@pearwood.info> wrote in message 
> news:554cd119$0$12977$c3e8da3$5496439d@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

[toc] | [prev] | [next] | [standalone]

#90218

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

[toc] | [prev] | [next] | [standalone]

#90222

"Gregory Ewing" <greg.ewing@canterbury.ac.nz> wrote in message 
news:cr5sc6FgfmiU1@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

[toc] | [prev] | [next] | [standalone]

#90152

On 05/08/2015 06:53 AM, Frank Millman wrote:
>
> "Steven D'Aprano" <steve+comp.lang.python@pearwood.info> wrote in message
> news:554c8b0a$0$12992$c3e8da3$5496439d@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

[toc] | [prev] | [next] | [standalone]

#90276

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@speranza.aioe.org>

[toc] | [prev] | [next] | [standalone]

#90277

"Johannes Bauer" <dfnsonfsduifb@gmx.de> wrote in message 
news:min3f0$2gh$1@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

[toc] | [prev] | [next] | [standalone]

#90279

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@speranza.aioe.org>

[toc] | [prev] | [next] | [standalone]

#90284

"Johannes Bauer" <dfnsonfsduifb@gmx.de> wrote in message 
news:min9t3$e56$1@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

[toc] | [prev] | [next] | [standalone]

#90168

"Dave Angel" <davea@davea.name> wrote in message 
news:554CA652.1000607@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

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web