Simple PIN Locker in Python

Question

I am fairly new to Python but I have been improving, and I wanted to try writing a simple program that simulates locker with multiple chambers (4) for storing "objects". I recently used lockers at a theme park with a similar functionality.

A chamber can be reserved by simply typing a unique PIN. In the real world, the PIN would be a unique ID like a credit card track, a finger print, a barcode, or RFID.

If the PIN is already associated with a chamber, the chamber is released and made available and the user is destroyed.

If the PIN is unique, and there are chambers available (not reserved), a chamber is reserved for the user under the unique PIN.

I hope the explanation makes sense. I am looking for some pointers and critique on code organization and flow, best practices, and use of classes, methods, etc.

You can see the code in action here.

class Locker(object):
 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
 def find_chamber_by_pin(self, pin):
 for chamber in self.chambers.values():
 if pin == getattr(chamber.user, 'pin', None):
 return chamber
 return 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
 def __len__(self):
 return len(self.chambers.keys())
 def __repr__(self):
 return '<LOCKER: {} of {} Available>:'.format(self.count_available(), len(self))
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
 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

• cremefraiche: "Be consistent in return statements. Either all return statements in a function should return an expression, or none of them should. If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None , and an explicit return statement should be present at the end of the function (if reachable). " -- PEP 8, Python Style Guide
• 200_success: @cremefraiche Please convert your comment to an answer.

Answer 1

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.

• gtalarico: Wow. This is great. Thank you so much for taking the time to give me this feedback. I really appreciate it. I will digests this and try to improve the using your feedback and post back. Thanks again!
• gtalarico: Thanks again @ymbirtt. I will post the revised code as an answer below. One more question: I added @ property for the available_chambers() mehod. What do you think about that? Any advantage besides not having to type () when getting that "property" Any rules of thumb of when to use getter method vs @ property method? Thanks!
• ymbirtt: Thanks for the kind words @gtalarico. Unfortunately, posting revisions to code in the same question tends to be discouraged - meta.codereview.stackexchange.com/questions/1763 - and self-answering with your revised code should adhere to the same standards as other peoples' answers, ie it should be a full review of the code rather than a refactor. As for the questions you've asked, I'll edit my answer now to add my thoughts.

Answer 2

Very impressive, good job. I have a couple of things to say:

1. Reduce whitespace.
2. Put brackets next to print.

Example:

print ('Locker Created: ', self)