Though I've got a lot to say, it's mostly just me pulling out stuff from my Python toybox, which let you write what you want, rather than how to get it. In general, this is a really clean module and there's nothing utterly heinous going on in there.
def __init__(self, capacity):
self.chambers = {}
print 'Locker Initialized.'
for n, chamber in enumerate(range(0, capacity)):
self.chambers[n] = Chamber(n)
print 'Locker Created: ', self
For starters, you don't need to use a dict for this, a list will do just fine. You're giving yourself a constant number of chambers, nothing new ever enters, and nothing ever gets deleted. You may have done this because you've got a C or Java background, know about linked lists, and are a bit worried about access times. In Python, a "list" is actually an array under the hood, so you actually gain by using one.
The range function by default goes from 0 to n-1, so adding a 0 parameter is needless here.
Enumerate will zip together an enumerable with incrementing integers. Range returns a list of incrementing integers, so calling enumerate(range(0,n)) gives you back [(0,0), (1,1), (2,2), ..., (n-1,n-1)] (effectively, it's a little subtler than that). I can't really think of a use case for this, and you don't actually use the second part of each tuple anyway, so you can strip that out.
Finally, comprehensions are a really useful way of generating lists more cleanly. Rather than having a loop that keeps appending things, describing how a result is achieved, you instead write an expression that states what the result is, which makes your intention a little clearer. I'd replace your __init__ with
def __init__(self, capacity):
print 'Locker Initialized.'
self.chambers = [Chamber(n) for n in range(capacity)]
print 'Locker Created: ', self
def find_chamber_by_pin(self, pin):
for chamber in self.chambers.values():
if pin == getattr(chamber.user, 'pin', None):
return chamber
return None
getattr is awesome, but when you use it with a constant string you don't gain a great deal. You know that chamber.user will always be either an instance of User or None, and in Python None is falsey, so you can replace your condition with if chamber.user and pin == chamber.user.pin:
I'd go a little further than this, though. Your loop is over the list of occupied chambers, so let's call it that...
def occupied_chambers(self):
return (chamber for chamber in self.chambers if self.chamber.occupied)
def find_chamber_by_pin(self, pin):
for chamber in self.occupied_chambers():
if chamber.user.pin == pin:
return chamber
return None
If you wanted to crush your number of lines down, you could also exploit next, but the resulting function starts to look a little chaotic. It's there if you like it:
def find_chamber_by_pin(self, pin):
return next((chamber for chamber in self.occupied_chambers()
if chamber.user.pin == pin), None)
def find_available_chamber(self):
for chamber in self.chambers.values():
if chamber.occupied is False:
return chamber
return None
def count_available(self):
counter = 0
for chamber in self.chambers.values():
if chamber.occupied is False:
counter += 1
return counter
There aren't all that many situations where you gain from comparing stuff to True and False. Rather than chamber.occupied is False, not chamber.occupied is preferred (but see later on).
These functions both iterate over the same thing, so abstract it out:
def available_chambers(self):
return [chamber for chamber in self.chambers if not chamber.occupied]
def find_available_chamber(self):
try:
return self.available_chambers()[0]
except IndexError:
return None
def count_available(self):
return len(self.available_chambers())
class Chamber(object):
def __init__(self, uid):
self.uid = uid
self.occupied = False
self.user = None
print 'Chamber Initialized: ', self
def reserve(self, user):
self.occupied = True
self.user = user
print 'Chamber Reserved: ', self
def release(self):
self.occupied = False
user = self.user
del user
self.user = None
print 'Chamber Released: ', self
First off, your release method deletes its user. This doesn't actually achieve anything. You can just set the user to None and the garbage collector will sort it out for you when it notices your object no longer references it.
Second, it seems like you want to keep an invariant that occupied is True if user is set and occupied is false if user is unset. For something like this, I'd use the property decorator which, among other things, lets you create properties that only exist based on other properties of your object. Half the time though we're not interested in whether or not the chamber is occupied, we want to know that it's available, so let's add that in, too
@property
def occupied(self):
return bool(self.user)
@property
def available(self):
return not self.occupied
Now we don't need to keep updating that occupied property any more! You can also go back and change your Locker.available_chambers() method to use this new property.
Putting it all together, I refactored your code into:
class Locker(object):
def __init__(self, capacity):
print 'Locker Initialized.'
self.chambers = [Chamber(n) for n in range(capacity)]
print 'Locker Created: ', self
@property
def occupied_chambers(self):
return [chamber for chamber in self.chambers if chamber.occupied]
@property
def available_chambers(self):
return [chamber for chamber in self.chambers if chamber.available]
def find_chamber_by_pin(self, pin):
return next((chamber for chamber in self.occupied_chambers
if chamber.user.pin == pin), None)
def find_available_chamber(self):
try:
return self.available_chambers[0]
except IndexError:
return None
def count_available(self):
return len(self.available_chambers)
def __len__(self):
return len(self.chambers)
def __repr__(self):
return '<LOCKER: {} of {} Available>:'.format(self.count_available(), len(self))
class Chamber(object):
def __init__(self, uid):
self.uid = uid
self.user = None
print 'Chamber Initialized: ', self
@property
def occupied(self):
return bool(self.user)
@property
def available(self):
return not self.occupied
def reserve(self, user):
self.user = user
print 'Chamber Reserved: ', self
def release(self):
self.user = None
print 'Chamber Released: ', self
def __repr__(self):
return '<CHAMBER {}:{}>:{}'.format(self.uid, self.occupied,
self.user)
class User(object):
def __init__(self, pin):
self.pin = pin
print 'User Created: ', self
def __repr__(self):
return '<USER PIN: {}>'.format(self.pin)
locker = Locker(4)
while True:
pin = raw_input('Locker Idle. Enter Unique PIN:')
chamber = locker.find_chamber_by_pin(pin)
if chamber:
chamber.release()
else:
chamber = locker.find_available_chamber()
if chamber:
user = User(pin)
chamber.reserve(user)
else:
print 'Chambers Full. Wait for relase.'
print 'New Status: ', locker
You asked about the advantages of using @property, and when it's appropriate to use it. I've not been able to dig out any "official" style guides on it, so what you're about to read is basically just my opinion. I should also point out that I'm a Ruby developer, too, so a lot of its idioms tend to bleed into my Python. Most notably, in Ruby you don't generally care whether you're fetching a property or calling a 0-parameter method.
Functionally, there's not really much difference between a @property-decorated function and a regular one that just takes no arguments. The difference is in the "feel" of your interface to anyone using it. If you're asking an object about its state, then you probably want a @property decorator. If you're asking it to do something, then you want a method. Getting a property should be fairly quick, and should (almost) never change the state of the object.
Also notable is that Python is not Java, so methods like def get_foo(self): return self._foo are never appropriate. If a caller might want to do something with foo, then instead consider writing a method that does it for them. Don't expect client code to repeatedly call getters and setters to mutate your object.
