1

I am coding a text adventure in python. I have a sheriff NPC that counts all item objects in player.inventory, and if item.name == "dead goblin" then the item is removed from the list, and goblins_taken += 1:

    if "sheriff" in player.action and "talk" in player.action and player.location == sheriff:
        goblins_taken = 0
        for item in player.inventory:
            if item.name == "dead goblin":
                goblins_taken += 1
                player.inventory.remove(item)
        write("The sheriff says, 'Good job. You have killed " + str(goblins_taken) + " goblins!")

The problem with this code was the fact that if the player killed 2 goblins, the sheriff would say that the player killed 1 goblin, not 2.

I tried to simplify the problem in IDLE and realized that it was an indexing problem:

>>> foo = ['a', 'b', 'c', 'd', 'e']
>>> foo[2]
'c' # With b
>>> foo.remove('b')
>>> foo[2]
'd' # Without b, index is shifted down

Is there some better method of removing items from a list than list.remove()? Or is there some way to fix this problem?

Tiskolin
  • 167
  • 17
  • Could you simple give dead goblins a special tag (i.e. 'dead') and then count 'dead'? – SuperStew Nov 21 '17 at 18:53
  • Related reading: [Remove items from a list while iterating](https://stackoverflow.com/q/1207406/953482), [Python: Removing list element while iterating over list \[duplicate\]](https://stackoverflow.com/q/6022764/953482), [Removing from a list while iterating over it](https://stackoverflow.com/q/6500888/953482) – Kevin Nov 21 '17 at 18:53
  • The simple way is to build a new list, but I also like John Machin's approach: do it backwards. https://stackoverflow.com/a/1207485/4014959 – PM 2Ring Nov 21 '17 at 18:55
  • I am not adding a `dead` tag to the goblins. Although I have considered it, I have other enemies based on the same `Enemy` class that the sheriff is not interested in which already have `if self.health <= 0` statements instead, so a `dead` tag would not be necessary. Thanks anyway! – Tiskolin Nov 21 '17 at 19:16

1 Answers1

2

You should never modify the list you are iterating through! You could solve it by creating a second list:

temp_inventory = list(player.inventory)
for item in player.inventory:
    if item.name == "dead goblin":
        goblins_taken += 1
        temp_inventory.remove(item)
player.inventory = temp_inventory

This I have tried and works:

a = [1,2,3,2,1]
b = list(a)
elems_removed = 0
for elem in a:
    if elem == 2:
        elems_removed += 1
        b.remove(elem)
print(a)
print(b)
a=b
print(a)
print(elems_removed)
mrCarnivore
  • 4,638
  • 2
  • 12
  • 29
  • 1
    @TemporalWolf: You are right. I have corrected my answer. Thanks for the warning. I had not tried the code for lack of runable code in the question. – mrCarnivore Nov 21 '17 at 18:59
  • You can make the temporary variable anonymous via: `for item in player.inventory[:]:` or `for item in list(player.inventory):` both of which make [shallow copies](https://docs.python.org/2/library/copy.html) to iterate over. – TemporalWolf Nov 21 '17 at 19:03
  • That is what I changed in my answer. – mrCarnivore Nov 21 '17 at 19:03
  • Seemed pretty reasonable to me. Unfortunately, when I tested this out my Raspberry Pi completely stalled, so I had to unplug it. Turned it back on and going to see what happened. – Tiskolin Nov 21 '17 at 19:04
  • There we go! I updated the code and it works. Thanks, mrCarnivore. – Tiskolin Nov 21 '17 at 19:07
  • Probably better to iterate over the copy and remove from the original, just in case there are other references to the original. Or at the end of the loop do `player.inventory[:] = temp_inventory`. This technique is ok for small lists, but it's better to avoid `.remove` because it's slow: it has to do a linear scan of the whole list until it finds a matching item to remove, as mentioned in the linked question. – PM 2Ring Nov 21 '17 at 19:08