2 
class Foo(object):
    def __init__(self,x):
        self.x = x
        self.is_bar = False
    def __repr__(self): return str(self.x)

class Bar(object):
    def __init__(self,l = []):
        self.l = l
    def add(self,o):
        self.l += [o]
    def __repr__(self): return str(self.l)

def foo_plus_foo(f1,f2):
    t = Bar()
    if not (f1.is_bar and f2.is_bar):
        f1.is_bar = True
        f2.is_bar = True
        t.add(f1)
        t.add(f2)
        print 'HERE'
    return t

if __name__ == '__main__':
    li = [Foo(1), Foo(2)]
    print foo_plus_foo(li[0],li[1])
    print foo_plus_foo(li[0],li[1])

UNEXPECTED OUTPUT:

HERE
[1, 2]
[1, 2]

EXPECTED OUTPUT:

HERE
[1, 2]
[]

What is happening? What did I do wrong? Why is python using old value? What do I do to avoid this?

Thanks!

Improve this question

4 Answers 4

Reset to default
5

Never. Do. This.

def __init__(self,l = []):

Never.

One list object is reused. And it's Mutable, so that each time it's reused, the one and only [] created in your method definition is updated.

Always. Do. This.

def __init__( self, l= None ):
    if l is None: l = []

That creates a fresh, new, unique list instance.

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

how about self.l = [] if l is None else l? will this create any problems?
@TheMachineCharmer: When you actually tried it, what did you observe?
@TheMachineCharmer: Then why ask?
I thought there might be some pitfalls I don't know of. Till date I was considering use of [] as default value perfectly fine. Thanks! :)
@S. Lott: Why never do this? If one knows the semantics of the l = [] parameter, I don't see why it should not be used on purpose.
|
4

You are defining l as having a default value of []. This is a classic Python pitfall.

class Bar(object):
    def __init__(self,l = []):

Default values are evaluated at definition time not run-time. It is evaluated only once.

So t=Bar() sets t.l to the very same list every time.

To fix this, change Bar to 

class Bar(object):
    def __init__(self,l = None):
        if l is None:
            l=[]
        self.l = l
    def add(self,o):
        self.l += [o]
    def __repr__(self): return str(self.l)
Improve this answer

3 Comments

but I am also creating new t = Bar() everytime.
Yes, but since you don't supply an argument to Bar, the same old list is set every time. This list is getting mutated with each run of foo_plus_foo.
Thanks!! I fell in the "pit". :D
1

The culprit is the l=[] defined in the Bar class definition. This list is instantiated once during class definition and is used as the default. Super dangerous!! I and many others have been burned by this one, trust me the scarring is deep.

Problematic use of mutable.

class Bar(object):
    def __init__(self,l = []):
        self.l = l
    def add(self,o):
        self.l += [o]
    def __repr__(self): return str(self.l)

Try using an immutable:

class Bar(object):
    def __init__(self,l = None):
        if l is None:
            self.l = []
        else:
            self.l = l
    def add(self,o):
        self.l += [o]
    def __repr__(self): return str(self.l)
Improve this answer

Comments

1

Others have explained the problem and suggested using l=None and an explicit test for it. I'd like to suggest a different approach:

class Bar(object):
    def __init__(self, l=[]):
        self.l = list(l)
        # and so on...

This guarantees a new blank list each time by default, and also assures that if a caller passes in a list of their own, you get a copy of that list rather than a reference to the caller's list. As an added benefit, callers can pass in anything that the list constructor can consume, such as a tuple or a string, and your code doesn't have to worry about that; it can just deal with a list.

If you just save a reference to the list the caller gives you, and later change the list named self.l, you may be inadvertently changing the list that was passed in, too (since both your class and the caller now have a reference to the same object). Similarly, if they change the list after calling your constructor, your "copy" will be changed too (since it's not actually a copy). Of course, it could be that this is behavior you want, but probably not in this case.

Although if you never manipulate the list (i.e. add, replace, or delete items), but only refer to it or replace it wholesale, copying it is usually a waste of time and memory.

The copy made using the list() constructor is shallow. That is, the list itself is a new object, but if the original list contains references to mutable objects (such as other lists or dictionaries, or instances of most other classes), the same problem can arise if you change those objects, because they are still shared between the lists. This is an issue less frequently than you might think, but if it is, you can perform a deep copy using the deepcopy() function in the copy module.

Improve this answer

1 Comment

I like this. In addition to receiving tuple or strings. Iterables and even generator expressions can be used. Alternatively self.l = iter(l) if you are simply going to loop through the input but don't want to walk the input immediately. This is not applicable to this specific question but still an interesting use of the pattern.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.