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 '[EMAIL PROTECTED]'.split('@')])"
--
http://mail.python.org/mailman/listinfo/python-list
