450 Pound Library Program

[bruno at modulix]

mwt wrote:
> So in a further attempt to learn some Python, I've taken the little
> Library program
> (http://groups.google.com/group/comp.lang.python/browse_thread/thread/f6a9ccf1bc136f84)
> I wrote and added several features to it. Readers now quit when they've
> read all the books in the Library. Books know how many times they've
> been read. Best of all, you can now create your own list of books to
> read!
> 
> Again, the point of all this is to get used to programming in Python.
> So although the program is trivial, any feedback on style, structure,
> etc. would be much appreciated. I'm a convert from Java,

Welcome !-)

> so I've
> probably got some unconscious Javanese in there somewhere. Help me get
> rid of it!

Well, apart from namingConventions (vs naming_conventions), I did not
spot too much javaism in your code.

> Here's the new, improved program:
> [code]
> #!/usr/bin/python
> # Filename: Library.py
> # author: mwt
> # Feb, 2006
> 
> import thread
> import time
> import threading
> import random
> 
> 
> 
> class Library2:

Old-style classes are deprecated, use new-style classes instead:
class Library2(object):

>     def __init__(self, listOfBooks, totalBooks):
>         self.stacks = listOfBooks

If its a collection of books, why not call it 'books' ?

>         self.cv = threading.Condition()
>         self.totalBooks = totalBooks

What is 'totalBooks' ?

>     def checkOutBook(self, readerName):
>         "'Remove book from the front of the list, block if no books are
> available'"

Why not using triple-quoted strings ?

>         self.cv.acquire()
>         while len(self.stacks) == 0:
>             self.cv.wait()
>             print "%s waiting for a book..." %readerName
>         book = self.stacks.pop(0)

This last line will crash (IndexError) on an empty list, and then the
resource may not be released... A first step would be to enclose this in
a try/finally block.

>         self.cv.release()
>         return book
> 
>     def returnBook(self, returnedBook):
>         "'put book at the end of the list, notify that a book is
> available'"
>         returnedBook.wasRead()
>         self.cv.acquire()
>         self.stacks.append(returnedBook)
>         self.cv.notify()
>         self.cv.release()

You have a recurring pattern in the last 2 methods: aquire/do
something/release. You could factor this out in a method decorator
(don't forget that functions and methods are objects too, so you can do
a *lot* of things with them).

> class Reader(threading.Thread):
> 
>     def __init__(self, library, name, readingSpeed, timeBetweenBooks):
>         threading.Thread.__init__(self)
or :
          super(Reader, self).__init__()

but this wouldn't make a big difference here

>         self.library = library
>         self.name = name
>         self.readingSpeed = readingSpeed
>         self.timeBetweenBooks = timeBetweenBooks
>         self.book = ""

You later user Reader.book to hold a reference to a Book object, so it
would be wiser to initialize it with None.

>         self.numberOfBooksRead = 0
> 
>     def run(self):
>         "'Keep checking out and reading books until you've read all in
> the Library'"
>         while  self.numberOfBooksRead < self.library.totalBooks:
>             self.book = self.library.checkOutBook(self.name)
>             print "%s reading %s" %(self.name, self.book.title),
>             time.sleep(self.readingSpeed)
>             self.numberOfBooksRead += 1
>             self.library.returnBook(self.book)
>             print "%s done reading %s" %(self.name, self.book.title),
>             print"Number of books %s has read: %d" %(self.name,
> self.numberOfBooksRead)
>             self.bookName = ""
>             time.sleep(self.timeBetweenBooks)
>         print "%s done reading." %self.name
> 
> class Book:
>     def __init__(self, author, title):
>         self.author = author
>         self.title = title
>         self.numberOfTimesRead = 0
>         #print "%s,%s" % (self.author, self.title),#print as books are
> loaded in
> 
>     def wasRead(self):
>         self.numberOfTimesRead += 1
>         print "Number of times %s has been read: %d" %(self.title,
> self.numberOfTimesRead)
> 
> if __name__=="__main__":
> 

You should define a main() function and call it from here.

>     print "\nWELCOME TO THE THURMOND STREET PUBLIC LIBRARY"
>     print "Checking which books are avialable...\n"
s/avialable/available/ !-)

>     try:
>         theBookFile = open("books.txt", "r")#Create your own list of
> books!

Filenames should not be hardcoded.

(Ok, you probably know this already, but I thought it would be better to
point this out for newbie programmers)

>         stacks = []#a place to put the books
>         for line in theBookFile.readlines():
>             L = line.split (",")  # a comma-delimited list

Mmm... may not be the best format. What if there are commas in the book
title ? Hint : there's a CSV module in the standard lib.

Also, by convention, UPPERCASE identifiers are considered as CONSTANTS

>             author = L[0]
>             bookName =  L[1]
>             newBook = Book(author, bookName)
>             stacks.append(newBook)

You can smash all this in a single line:
       stacks = [Book(line.split(",", 1) for line in theBookFile]

>         theBookFile.close()
>     except IOError:
>         print "File not found!"

An IOError can result from a lot of different reasons (like user not
having read access on the file). So don't assume the file was not found:

       except IOError, e:
          print "Could not open file %s : %s" % ('books.txt', e)

You may also want to exit here...

>     #string = "How many books would you like in the Library?[1-" +
> str(len(stacks)) + "]"
>     totalBooks = input("How many books would you like in the
> Library?[1-" + str(len(stacks)) + "]")

1/ use raw_input() and *validate* user data

2/ use string formatting:
      prompt = "How many books would you like in the "
               " Library? [1-%d]" % len(stacks)

>     stacks[totalBooks: len(stacks)] = []
      stacks = stacks[:totalBooks]

May be less efficient, but much more readable IMHO.

Also, the naming is somewhat misleading. Something along the line of
numberOfBooks or booksCount would probably be better.

>     print "Number of books in the Library is:", len(stacks)

You've called len(stacks) 3 times in 3 lines.

>     library = Library2(stacks, totalBooks)

What's the use of totalBooks here ? The Library2 class can figure it out
by itself, so it's useless - and it goes against DRY and SPOT.

hint: given the use of Library.totalBook, you'd better make it a
computed attribute

class LibraryN(object):
  booksCount = property(fget=lambda self: len(self.books))

>     readers = input("\nHow many readers would you like?")

idem. Also, something like readersCount would be less misleading (I
first thought it was a collection of Reader objects)

>     print "Number of readers is:", readers, "\n"
>     for i in range(0,readers):
>        newReader = Reader(library, "Reader" + str (i),
> random.randint(1,7), random.randint(1,7))

performance hint: names are first looked up in the local namespace:

      randint = random.randint
      for i in range(0, readers):
         newReader = Reader(library,
                            "Reader%d" % i,
  			     randint(1,7),
                             randint(1,7))

>        newReader.start()

My 2 cents...

(snip)

-- 
bruno desthuilliers
python -c "print '@'.join(['.'.join([w[::-1] for w in p.split('.')]) for
p in 'onurb at xiludom.gro'.split('@')])"