6

I'm really stuck on why the following code block 1 result in output 1 instead of output 2?

Code block 1:

class FruitContainer:
       def __init__(self,arr=[]):
           self.array = arr
       def addTo(self,something):
           self.array.append(something)
       def __str__(self):
           ret = "["
           for item in self.array:
               ret = "%s%s," % (ret,item)
           return "%s]" % ret

arrayOfFruit = ['apple', 'banana', 'pear']
arrayOfFruitContainers = []

while len(arrayOfFruit) > 0:
   tempFruit = arrayOfFruit.pop(0)
   tempB = FruitContainer()
   tempB.addTo(tempFruit)
   arrayOfFruitContainers.append(tempB)

for container in arrayOfFruitContainers:
   print container 

**Output 1 (actual):**
[apple,banana,pear,]
[apple,banana,pear,]
[apple,banana,pear,]

**Output 2 (desired):**
[apple,]
[banana,]
[pear,]

The goal of this code is to iterate through an array and wrap each in a parent object. This is a reduction of my actual code which adds all apples to a bag of apples and so forth. My guess is that, for some reason, it's either using the same object or acting as if the fruit container uses a static array. I have no idea how to fix this.

Improve this question
1
  • 1
    Not an answer to your question, but also noteworthy: "while len(arrayOfFruit) > 0:" is equivalent to "while arrayOfFruit:". The latter is preferable, according to the Python Style Guide at least. Commented Oct 31, 2009 at 17:27

4 Answers 4

Reset to default
8

You should never use a mutable value (like []) for a default argument to a method. The value is computed once, and then used for every invocation. When you use an empty list as a default value, that same list is used every time the method is invoked without the argument, even as the value is modified by previous function calls.

Do this instead:

def __init__(self,arr=None):
    self.array = arr or []
Improve this answer
Sign up to request clarification or add additional context in comments.

10 Comments

I really do not like to see you testing for None by seeing if the value evaluates false. It is better to use an is None test. A caller could legally pass an empty list in for the initializer, and your code would discard that empty list and make a fresh empty list... this would only come up if someone was trying to make several instances of the class all share the same initially-empty list, I guess, but it is possible. In any event, using is to test for None is a good habit.
Oh, a better example for how your code could fail: someone could make a subclass of list with some desired extra properties, pass that as the initializer, and if it evaluated false your code would discard it in favor of an empty, standard list.
Better still, leave arr=[] and use self.array = list(arr). As it stands, the user cannot pass in a generator, or a tuple, to the constructor without the class breaking.
@chrispy, that is a bad idea. If the user wants to expand an iterator to a list, the user can call FruitContainer(list(iterable)). But what if the user wants to pass in a subclass of a list, with side-effect logging or something? Your list(arr) would get rid of the subclassed object and force it to be a boring list. There is a standard, time-tested Python idiom for this, and it is the test for is None followed by storing the provided argument if it was not None.
@steveha, how likely is it that the user wants to concern himself with the internal storage of the class?
|
2

Your code has a default argument to initialize the class. The value of the default argument is evaluated once, at compile time, so every instance is initialized with the same list. Change it like so:

def __init__(self, arr=None):
    if arr is None:
        self.array = []
    else:
        self.array = arr

I discussed this more fully here: How to define a class in Python

Improve this answer

Comments

1

As Ned says, the problem is you are using a list as a default argument. There is more detail here. The solution is to change __init__ function as below: 

       def __init__(self,arr=None):
           if arr is not None:
               self.array = arr
           else:
               self.array = []
Improve this answer

Comments

0

A better solution than passing in None — in this particular instance, rather than in general — is to treat the arr parameter to __init__ as an enumerable set of items to pre-initialize the FruitContainer with, rather than an array to use for internal storage:

class FruitContainer:
  def __init__(self, arr=()):
    self.array = list(arr)
  ...

This will allow you to pass in other enumerable types to initialize your container, which more advanced Python users will expect to be able to do:

myFruit = ('apple', 'pear') # Pass a tuple
myFruitContainer = FruitContainer(myFruit)
myOtherFruit = file('fruitFile', 'r') # Pass a file
myOtherFruitContainer = FruitContainer(myOtherFruit)

It will also defuse another potential aliasing bug:

myFruit = ['apple', 'pear']
myFruitContainer1 = FruitContainer(myFruit)
myFruitContainer2 = FruitContainer(myFruit)
myFruitContainer1.addTo('banana')
'banana' in str(myFruitContainer2)

With all other implementations on this page, this will return True, because you have accidentally aliased the internal storage of your containers.

Note: This approach is not always the right answer: "if not None" is better in other cases. Just ask yourself: am I passing in a set of objects, or a mutable container? If the class/function I'm passing my objects in to changes the storage I gave it, would that be (a) surprising or (b) desirable? In this case, I would argue that it is (a); thus, the list(...) call is the best solution. If (b), "if not None" would be the right approach.

Improve this answer

3 Comments

Hi there, I guess I am "someone". I have no objection to coding this example this way. In this example, you are passing in fruit to be added to internal storage in the FruitContainer class. The user isn't so much passing in a list object as initializing the container with some fruit. Now, in the general case, I don't think it is a good idea to silently coerce the things the user provides. I think the reason we were talking past each other is that I was focused on the general case and you were thinking of this very specific case. In general, don't break duck typing by coercing types.
Excellent. I've rewritten my last paragraph to reflect this. Thanks for taking the time to make your point clearer to me!
I'm just sorry I didn't figure out where you were coming from sooner. I was in full-on pedant mode, I guess. :-)

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.