Two removals with the same rule and different timing. One of them can leave the stock half-adjusted.
# Change, then notice
self.stock[item] -= count
if self.stock[item] < 0:
raise ValueError('not enough stock')
# the exception escapes and the stock is already negative
# Notice, then change
if count > self.stock.get(item, 0):
raise ValueError('not enough stock')
self.stock[item] -= count
# And zero means gone, not zero
if self.stock[item] == 0:
del self.stock[item]
- A raise in the middle of a change leaves the change half-done
Python does not undo anything when an exception propagates, so any modification made before the check survives it. The caller catches the ValueError, assumes nothing happened, and carries on with corrupted state. Validate first — and where you cannot, build the new state separately and swap it in at the end.
- Absent and zero should not both be possible
If an item can be stored with a count of zero, then 'is this item in stock' has two different answers depending on how it got there, and every listing has to filter. Deleting the entry when it empties means there is exactly one representation of 'we have none', which is one fewer thing for every reader to remember.
- quantity() answers for items you have never heard of
Returning 0 rather than raising for an unknown item is what lets callers write `if inventory.quantity(x) < reorder_level` without a membership check first. It is the same judgement as dict.get with a default — and it is only safe because zero and absent genuinely mean the same thing here.
One dictionary inside, mapping item to count. Both methods start by validating the count, and remove also has to check there is enough — before it changes anything.
quantity() is self._stock.get(item, 0), and remove can use it: if count > self.quantity(item), raise. After subtracting, delete the key when it reaches zero.
Both methods need the same 'count must be positive' check, so give it a small helper or repeat the two lines deliberately. total is sum(self._stock.values()); __len__ and __iter__ delegate straight to the dictionary, which is what keeps the insertion order.
class Inventory: def __init__(self): self._stock = {} def add(self, item, count): if count <= ____: raise ValueError(f’count must be positive, got {count}') self._stock[item] = self.quantity(item) + count def remove(self, item, count): if count <= 0: raise ValueError(f’count must be positive, got {count}') held = self.____(item) if count > held: raise ____(f’only {held} of {item!r} in stock') if count == held: ____ self._stock[item] else: self._stock[item] = held - count def quantity(self, item): return self._stock.____(item, 0) @property def total(self): return sum(self._stock.values()) def __len__(self): return len(self._stock) def __iter__(self): return iter(self._stock)
class Inventory:
"""Stock levels that cannot go negative."""
def __init__(self):
self._stock = {}
def add(self, item, count):
self._check(count)
self._stock[item] = self.quantity(item) + count
def remove(self, item, count):
self._check(count)
held = self.quantity(item)
# Everything is checked before anything is changed, so a refused
# removal leaves the stock exactly as it was.
if count > held:
raise ValueError(
f"cannot remove {count} of {item!r}: only {held} in stock"
)
if count == held:
del self._stock[item]
else:
self._stock[item] = held - count
def quantity(self, item):
return self._stock.get(item, 0)
@property
def total(self):
return sum(self._stock.values())
def __len__(self):
return len(self._stock)
def __iter__(self):
return iter(self._stock)
@staticmethod
def _check(count):
if count <= 0:
raise ValueError(f"count must be positive, got {count}")
self.quantity(item) + count is 0 + count for an item nobody has, and the existing total plus count otherwise — one line covering both. Reaching into self._stock with a membership check would be two branches saying the same thing, and the public method already knows the answer.
_stock is a convention, not a lock — anybody can reach in and set it to nonsense. What it buys is that nobody does so *by accident*, and that every legitimate path goes through the two methods where the rules live. The rules are only as good as the number of doors into the state.
inventory.total reads like an attribute because it behaves like one: cheap, derived, no side effects. The moment it needed an argument — a total for one category, say — it would have to become a method, and leaving it as a property while it quietly got expensive would be the worse outcome.
For anything with real inventory semantics — reservations, stock across warehouses, concurrent orders — this is a toy, and the correct place for the constraint is the database: a CHECK (quantity >= 0) and a transaction, so two simultaneous removals cannot both pass the check. Nothing here is safe against threads either; the read in quantity() and the write that follows it are two separate steps. Use this shape when the state lives in one process and one thread, which covers a surprising amount of scripting, simulation and game code.
- Add a move(item, count, other) that transfers stock between two inventories and cannot lose items if the second step fails.
- Track a reserved count alongside the held count, so stock can be committed to an order without leaving the shelf.
- Add __contains__ and decide whether an item with zero stock is in the inventory — the answer should already be settled by the contract.