Here are 3 versions of a method from my inventory class that adds an item to the given inventory (or increases the items quantity). From a Python perspective, which approach would be best? Each method is followed by a sample of how I would use it in practice. I've been pretty interested in code design/architecture lately and I've been trying to get better at Python, so it may seem like a silly question but I'm just curious what you people think. Currently, I'm using approach #1, but I find it misleading that you could add an item and not actually have an item added. And if there's another approach not listed here that would be good please let me know.

#1 prevent adding item if is_full():
def add_item(self, item):
        i = next((i for i in self.items if i.tag == item.tag), None) #Duplicate item...
        if i is not None:
            i.quantity += item.quantity
        elif not self.is_full():
            self.items.append(item)

player_inventory.add_item(item)

#2 throw exception if is_full() and handle outside method:
def add_item(self, item):
        i = next((i for i in self.items if i.tag == item.tag), None)
        if i is not None:
            i.quantity += item.quantity
        elif not self.is_full():
            self.items.append(item)
        else: 
            raise InvetoryFullException("Inventory can only hold 8 items.")

try:			
	player_inventory.add_item(item)
except InvetoryFullException:
	pass
			
#3 check if inventory is_full() outside of method:
def add_item(self, item):
        i = next((i for i in self.items if i.tag == item.tag), None)
        if i is not None:
            i.quantity += item.quantity
        else:
            self.items.append(item)

if not player_inventory.is_full():
	player_inventory.items.append(item)

Since the items are referred to by their tag, then it would be easier to use an associative array, such as `dict` or `collections.OrderedDict`.  Then, checking if the inventory is full is only a matter of checking the number of tags it has:

items = dict()

def is_full(self):
    return len(self.items) >= 8

You are correct in that it is misleading to add an item and not actually have an item added.  This is why it is important to signal if an action succeeded and (optionally) provide other ways to determine if an action will succeed.  For example:

class Inventory:
    max_inventory_size = 8

    def __init__(self):
        self.items = dict()

    @property
    def full(self):
        return len(self.items) >= self.max_inventory_size


    def can_accept(self, item):
        return item.tag in self.items or not self.full


    def try_add_item(self, item):
        if not self.can_accept(item):
            return False

        self.add_item(item)
        return True


    def add_item(self, item):
        target = self.items.get(item.tag, None)

        if target is not None:
            target.quantity += item.quantity

        else:
            assert not self.full, "Inventory full"
            self.items[item.tag] = item


# and then: (1)
# expect this action to succeed, otherwise crash hard
inv.add_item(item)


# or: (2)
# lazily attempt action, not caring if it fails
if inv.try_add_item(item):
    world.remove(item)


# or: (3)
# don’t attempt action, but do check if it is possible
for shop_entry in shop_menu:
    if not inv.can_accept(shop_entry.item):
        shop_entry.enabled = False

9 hours ago, fastcall22 said:

items = dict()

Makes sense to use a dict actually, not sure why I went with a list.

9 hours ago, fastcall22 said:

@property def full(self): return len(self.items) >= self.max_inventory_size

Curious to why you decided to make this a property?

Also, what's your take on using an exception? As in my second example.

Silently ignoring a request is a recipe for making buggy code, so I would strongly suggest you don't do that. Python also recommends that in the "Zen of Python" (type "import this" after the ">>>" prompt). "Explicit is better than implicit."

Making it testable before-hand, or returning "False" if it fails is a simple solution here. For something as simple as a full inventory, it's likely also enough. If you cannot store the item, something else must be done with it, and likely the best place to handle that problem is at the same spot in the code as where you tried (and failed) to insert the item into the inventory. An obvious thing to do is to put the item back where the player found it, for example.

An exception is a stronger failure, it is typically used when you're in real trouble. A file you loaded last time suddenly disappeared for example. The "return False" trick is a valid solution here too, but one level higher, there isn't much you can do about a missing file, so the only thing you can do is again "return False" to the next higher level, and so on, until you are somewhere near the top-level, where you know what files exist, you scratch the missing file, and you pick a new file to load.

When you raise an exception when you find the file missing instead, it does the above in one big step, it repeatedly exits the function, checks for a "catch", and if it doesn't exist, immediately exits that function too, etc, until it finds a "catch" statement that matches, or it reaches the main program (at which point the entire program ends). As such the raise is a substitute for the whole chain of
 

result = subfunction()
if not result: return False

that you'd have to write otherwise. It's a sort-of super-return.

Exceptions are quite controversial, there are disadvantages to them as well. Since you make big jumps from some innner-inner function back to some much higher function, all the variables that you have created and computations you have done in the functions between "raise" and "catch" are lost. You may not want that.

On 14/07/2017 at 5:38 AM, fastcall22 said:

Since the items are referred to by their tag, then it would be easier to use an associative array, such as `dict` or `collections.OrderedDict`.  Then, checking if the inventory is full is only a matter of checking the number of tags it has:

items = dict()

def is_full(self):
    return len(self.items) >= 8
    
...etc...

 

Slight amendment here; I would expect that to be more like:

def __init__():
    self.items = dict()
    
... other methods here, etc ...

Apart from that, I completely agree with this approach, and that the ability to add to the inventory should be checked explicitly with a function rather than handled with an exception. Exceptions are best saved for situations where it's not practical to easily check for the problem or to easily handle the problem where it's discovered.

On 7/15/2017 at 6:26 AM, Kylotan said:

Slight amendment here; I would expect that to be more like:

def __init__():
    self.items = dict()
    
... other methods here, etc ...

 

Apart from that, I completely agree with this approach, and that the ability to add to the inventory should be checked explicitly with a function rather than handled with an exception. Exceptions are best saved for situations where it's not practical to easily check for the problem or to easily handle the problem where it's discovered.

Yes, I am initializing that in the __init__ method. And makes sense, I decided to remove the is_full() check from each method and have the inventory size enforced outside of the inventory class. This also removed a lot of clutter from my methods which is good.

I was wondering if you can clarify when to use properties? I tend to avoid them because I feel like they are misleading.

For example,

@property
    def full(self):
        return len(self.items) >= self.max_inventory_size

Doesn't this hide the fact that this is a method? Would a user not think this a regular variable and possibly try to assign to it? I have other methods labeled get and set throughout my code base that changes behavior within different classes. Would it be misleading to convert those to properties?

Another example, would it be to misleading to make this a next_sprite property?

def get_next_sprite(self):
        if self.current_count == self.max_count: #Time to switch sprites...
            self.current_count = 0
            self.current_sprite = next(self.sprite_reel)
        else: #Increment and return the same sprite from last frame...
            self.current_count += 1
        return self.current_sprite

Properties are intended to hide the fact that they are a method; whether you think that is a good idea or not is up to you. A reasonable middle-ground would be to use a property only where the hidden side-effects make sense in some way. I write entire Python programs without ever defining properties and I'm happy with that. And I see some popular Python packages that use properties in a way I don't agree with, but the world doesn't end. It's up to you.

Examples of 'good' candidates for properties, in my opinion, might be methods that fix up the incoming value on a set-property (e.g. changing a string to a unicode, or an integer to a float), or methods which wrap a more complex comparison (e.g. the 'is_full' concept)

And an example of a 'bad' candidate for properties might be methods that look like getters but which change the object state, e.g. by iterating over some internal collection. (So personally, I wouldn't like a 'next_sprite' property.)

I wouldn't worry about users wrongly thinking they can assign to a property that is read-only; Python code needs to be well-documented to be usable anyway due to the lack of type-checking, so you just need to note such properties accordingly.

On 7/17/2017 at 5:13 AM, Kylotan said:

Examples of 'good' candidates for properties, in my opinion, might be methods that fix up the incoming value on a set-property (e.g. changing a string to a unicode, or an integer to a float), or methods which wrap a more complex comparison (e.g. the 'is_full' concept)

If I see no reason for something to be a property over a method should I favor making it a property or a method? For example, I can see no benefit in doing a "full" property over a "is_full() method", other than not having to write (). I can understand if full started as a regular variable and was expanded to a property later, but it wasn't. So in that case what would you say?

If you don't want to use properties, then don't. They just exist for convenience, to allow you to create things that act much like regular variables but which require a little extra logic.