450 Pound Library Program

Collapse
This topic is closed.
X
X
 
  • Time
  • Show
Clear All
new posts
  • mwt

    #1

    450 Pound Library Program

    So in a further attempt to learn some Python, I've taken the little
    Library program
    (http://groups.google.com/group/comp....a9ccf1bc136f84)
    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, so I've
    probably got some unconscious Javanese in there somewhere. Help me get
    rid of it!

    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:
    def __init__(self, listOfBooks, totalBooks):
    self.stacks = listOfBooks
    self.cv = threading.Condition()
    self.totalBooks = totalBooks
    
    def checkOutBook(self, readerName):
    "'Remove book from the front of the list, block if no books are
    available'"
    self.cv.acquire()
    while len(self.stacks) == 0:
    self.cv.wait()
    print "%s waiting for a book..." %readerName
    book = self.stacks.pop(0)
    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()
    
    class Reader(threading.Thread):
    
    def __init__(self, library, name, readingSpeed, timeBetweenBooks):
    threading.Thread.__init__(self)
    self.library = library
    self.name = name
    self.readingSpeed = readingSpeed
    self.timeBetweenBooks = timeBetweenBooks
    self.book = ""
    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__":
    
    print "\nWELCOME TO THE THURMOND STREET PUBLIC LIBRARY"
    print "Checking which books are avialable...\n"
    try:
    theBookFile = open("books.txt", "r")#Create your own list of
    books!
    stacks = []#a place to put the books
    for line in theBookFile.readlines():
    L = line.split (",")  # a comma-delimited list
    author = L[0]
    bookName =  L[1]
    newBook = Book(author, bookName)
    stacks.append(newBook)
    theBookFile.close()
    except IOError:
    print "File not found!"
    #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)) + "]")
    stacks[totalBooks: len(stacks)] = []
    print "Number of books in the Library is:", len(stacks)
    library = Library2(stacks, totalBooks)
    readers = input("\nHow many readers would you like?")
    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))
    newReader.start()
    And here's a handy text file of books for you, so you don't have to
    make your own:
    Code:
    Conrad, Heart of Darkness
    Kafka, Die Verwandlung
    Hemingway, For Whom the Bell Tolls
    James Joyce, Dubliners
    Moliere, Cyrano de Bergerac
    William Golding, Lord of the Flies
    Dostoevski, Crime and Punishment
    Cervantes, Don Quixote
    Camus, L'Etranger
    Tolstoy, War and Peace
    Poe, Tales
    Faulkner, The Sound and the Fury
    Orwell, 1984
    Fitzgerald, The Great Gatsby
    Steinbeck, The Grapes of Wrath
    Huxley, Brave New World
    Twain, The Adventures of Huckleberry Finn
    Mann, Der Tod in Venedig
    Kesey, Sometimes a Great Notion
    Pynchon, Gravity's Rainbow
    McEwan, The Cement Garden
    Márquez, Cien Años de Soledad
    Salinger, The Catcher in the Rye
    Miltion, Paradise Lost
    Chapman et al , The Pythons
  • Magnus Lycka

    #2
    Re: 450 Pound Library Program

    just a few style notes...
    [color=blue]
    > def checkOutBook(se lf, readerName):
    > "'Remove book from the front of the list, block if no books are
    > available'"[/color]

    I don't understand what "' is supposed to imply. If you
    meant to use triple quoting, you need to use ''' or """.
    Then the string can contain line breaks. (Perhaps you have
    a really stupid email client/news client/editor/whatever that
    replaces ''' with "'? Please loose that or use """.)

    Also, please use lines that are short enough not to wrap
    around. Line wrapping makes the code very ugly. Do like this:

    def checkOutBook(se lf, readerName):
    """Remove book from the front of the list, block if no
    books are available."""

    [...][color=blue]
    > for line in theBookFile.rea dlines():[/color]

    In modern Python you simply write:
    for line in theBookFile:
    [color=blue]
    > L = line.split (",") # a comma-delimited list
    > author = L[0]
    > bookName = L[1][/color]

    Why bother with L? The follwing is as clear I think, and solves
    the problem of commas in the title. Also, don't put a space between
    the callable and the parenthesis please. See the Python style guide,
    PEP 008.

    author, bookName = line.split(",", 2)

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

    Again, don't make the lines so long. You don't have to do that.
    You can break lines freely inside (), {} and [], and adjacent
    string literals are automatically concatenated.

    totalBooks = input("How many books would you like in "
    "the Library?[1-%d]" % len(stacks))

    Comment

    • bruno at modulix

      #3
      Re: 450 Pound Library Program

      mwt wrote:[color=blue]
      > So in a further attempt to learn some Python, I've taken the little
      > Library program
      > (http://groups.google.com/group/comp....a9ccf1bc136f84)
      > 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,[/color]

      Welcome !-)
      [color=blue]
      > so I've
      > probably got some unconscious Javanese in there somewhere. Help me get
      > rid of it![/color]

      Well, apart from namingConventio ns (vs naming_conventi ons), I did not
      spot too much javaism in your code.
      [color=blue]
      > 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:[/color]

      Old-style classes are deprecated, use new-style classes instead:
      class Library2(object ):
      [color=blue]
      > def __init__(self, listOfBooks, totalBooks):
      > self.stacks = listOfBooks[/color]

      If its a collection of books, why not call it 'books' ?
      [color=blue]
      > self.cv = threading.Condi tion()
      > self.totalBooks = totalBooks[/color]

      What is 'totalBooks' ?
      [color=blue]
      > def checkOutBook(se lf, readerName):
      > "'Remove book from the front of the list, block if no books are
      > available'"[/color]

      Why not using triple-quoted strings ?
      [color=blue]
      > self.cv.acquire ()
      > while len(self.stacks ) == 0:
      > self.cv.wait()
      > print "%s waiting for a book..." %readerName
      > book = self.stacks.pop (0)[/color]

      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.
      [color=blue]
      > self.cv.release ()
      > return book
      >
      > def returnBook(self , returnedBook):
      > "'put book at the end of the list, notify that a book is
      > available'"
      > returnedBook.wa sRead()
      > self.cv.acquire ()
      > self.stacks.app end(returnedBoo k)
      > self.cv.notify( )
      > self.cv.release ()[/color]

      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).
      [color=blue]
      > class Reader(threadin g.Thread):
      >
      > def __init__(self, library, name, readingSpeed, timeBetweenBook s):
      > threading.Threa d.__init__(self )[/color]
      or :
      super(Reader, self).__init__( )

      but this wouldn't make a big difference here
      [color=blue]
      > self.library = library
      > self.name = name
      > self.readingSpe ed = readingSpeed
      > self.timeBetwee nBooks = timeBetweenBook s
      > self.book = ""[/color]

      You later user Reader.book to hold a reference to a Book object, so it
      would be wiser to initialize it with None.
      [color=blue]
      > self.numberOfBo oksRead = 0
      >
      > def run(self):
      > "'Keep checking out and reading books until you've read all in
      > the Library'"
      > while self.numberOfBo oksRead < self.library.to talBooks:
      > self.book = self.library.ch eckOutBook(self .name)
      > print "%s reading %s" %(self.name, self.book.title ),
      > time.sleep(self .readingSpeed)
      > self.numberOfBo oksRead += 1
      > self.library.re turnBook(self.b ook)
      > print "%s done reading %s" %(self.name, self.book.title ),
      > print"Number of books %s has read: %d" %(self.name,
      > self.numberOfBo oksRead)
      > self.bookName = ""
      > time.sleep(self .timeBetweenBoo ks)
      > print "%s done reading." %self.name
      >
      > class Book:
      > def __init__(self, author, title):
      > self.author = author
      > self.title = title
      > self.numberOfTi mesRead = 0
      > #print "%s,%s" % (self.author, self.title),#pr int as books are
      > loaded in
      >
      > def wasRead(self):
      > self.numberOfTi mesRead += 1
      > print "Number of times %s has been read: %d" %(self.title,
      > self.numberOfTi mesRead)
      >
      > if __name__=="__ma in__":
      >[/color]

      You should define a main() function and call it from here.
      [color=blue]
      > print "\nWELCOME TO THE THURMOND STREET PUBLIC LIBRARY"
      > print "Checking which books are avialable...\n"[/color]
      s/avialable/available/ !-)
      [color=blue]
      > try:
      > theBookFile = open("books.txt ", "r")#Create your own list of
      > books![/color]

      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)
      [color=blue]
      > stacks = []#a place to put the books
      > for line in theBookFile.rea dlines():
      > L = line.split (",") # a comma-delimited list[/color]

      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
      [color=blue]
      > author = L[0]
      > bookName = L[1]
      > newBook = Book(author, bookName)
      > stacks.append(n ewBook)[/color]

      You can smash all this in a single line:
      stacks = [Book(line.split (",", 1) for line in theBookFile]
      [color=blue]
      > theBookFile.clo se()
      > except IOError:
      > print "File not found!"[/color]

      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...
      [color=blue]
      > #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) ) + "]")[/color]

      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)
      [color=blue]
      > stacks[totalBooks: len(stacks)] = [][/color]
      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.
      [color=blue]
      > print "Number of books in the Library is:", len(stacks)[/color]

      You've called len(stacks) 3 times in 3 lines.
      [color=blue]
      > library = Library2(stacks , totalBooks)[/color]

      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.totalBo ok, you'd better make it a
      computed attribute

      class LibraryN(object ):
      booksCount = property(fget=l ambda self: len(self.books) )
      [color=blue]
      > readers = input("\nHow many readers would you like?")[/color]

      idem. Also, something like readersCount would be less misleading (I
      first thought it was a collection of Reader objects)
      [color=blue]
      > 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))[/color]

      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))
      [color=blue]
      > newReader.start ()[/color]

      My 2 cents...

      (snip)

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

      Comment

      • bruno at modulix

        #4
        Re: 450 Pound Library Program

        Magnus Lycka wrote:[color=blue]
        > just a few style notes...
        >[/color]
        (snip)[color=blue]
        >
        > Why bother with L? The follwing is as clear I think, and solves
        > the problem of commas in the title. Also, don't put a space between
        > the callable and the parenthesis please. See the Python style guide,
        > PEP 008.
        >
        > author, bookName = line.split(",", 2)[color=green][color=darkred]
        >>> "toto, tata, tutu".split("," , 2)[/color][/color][/color]
        ['toto', ' tata', ' tutu'][color=blue][color=green][color=darkred]
        >>>[/color][/color][/color]

        You want line.split(',', 1) here.



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

        Comment

        • Sion Arrowsmith

          #5
          Re: 450 Pound Library Program

          mwt <michaeltaft@gm ail.com> wrote:[color=blue]
          > while len(self.stacks ) == 0:[/color]

          To (kind of) repeat myself, the idiomatic Python would be:

          while not self.stacks:

          An empty list is considered to be false, hence testing the list
          itself is the same as testing len(l) > 0 .

          As someone else has noticed, you're using len() an awful lot
          when you don't need to.

          --
          \S -- siona@chiark.gr eenend.org.uk -- http://www.chaos.org.uk/~sion/
          ___ | "Frankly I have no feelings towards penguins one way or the other"
          \X/ | -- Arthur C. Clarke
          her nu becomeþ se bera eadward ofdun hlæddre heafdes bæce bump bump bump

          Comment

          • plahey@alumni.caltech.edu

            #6
            Re: 450 Pound Library Program

            Ok,

            I give up. DRY = Don't Repeat Yourself (google Pragmatic Programmers)
            but SPOT? Google is little help here, SPOT is too common a word.

            Thanks!

            Comment

            • Magnus Lycka

              #7
              Re: 450 Pound Library Program

              plahey@alumni.c altech.edu wrote:[color=blue]
              > Ok,
              >
              > I give up. DRY = Don't Repeat Yourself (google Pragmatic Programmers)
              > but SPOT? Google is little help here, SPOT is too common a word.[/color]

              The only SPOT I worked with (as I know of) was SPOT4
              (Le Systeme Pour 'l Observation de la Terre) but that's
              probably not it...

              Single Point Of Truth? It seems some claim that DRY and
              SPOT are the same things.

              Comment

              • bruno at modulix

                #8
                Re: 450 Pound Library Program

                plahey@alumni.c altech.edu wrote:[color=blue]
                > Ok,
                >
                > I give up. DRY = Don't Repeat Yourself (google Pragmatic Programmers)
                > but SPOT? Google is little help here, SPOT is too common a word.[/color]

                Single Point Of Truth (or Single Point Of Transformation)

                And before you mention it, yes, it's *almost* the same thing as DRY !-)


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

                Comment

                • bruno at modulix

                  #9
                  Re: 450 Pound Library Program

                  Magnus Lycka wrote:[color=blue]
                  > plahey@alumni.c altech.edu wrote:
                  >[color=green]
                  >> Ok,
                  >>
                  >> I give up. DRY = Don't Repeat Yourself (google Pragmatic Programmers)
                  >> but SPOT? Google is little help here, SPOT is too common a word.[/color]
                  >
                  >
                  > The only SPOT I worked with (as I know of) was SPOT4
                  > (Le Systeme Pour 'l Observation de la Terre) but that's
                  > probably not it...[/color]

                  Probably not !-)
                  [color=blue]
                  > Single Point Of Truth?[/color]

                  And the winner is... <your-name-here />
                  [color=blue]
                  > It seems some claim that DRY and
                  > SPOT are the same things.
                  > http://www.artima.com/cppsource/reducepnp3.html[/color]

                  Well, they are at least very closely related !-)

                  And I of course forgot OnceAndOnlyOnce ...

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

                  Comment

                  • mwt

                    #10
                    Re: 450 Pound Library Program

                    A million thanks for in-depth critique. I look forward to figuring out
                    half of what you're talking about ;)

                    Comment

                    • mwt

                      #11
                      Re: 450 Pound Library Program

                      BTW - I can't find any code examples of how to interrupt a beast like
                      this. A regular ctrl-c doesn't work, since the separate threads just
                      keep executing. Is this a case where you need to iterate through all
                      threads and stop each one individually? How would that look?

                      Comment

                      Working...