21
\$\begingroup\$

I have been learning programming in Python recently and I got a task to program a library in OOP. You can add books to a library, search for a specific book according to the ISBN, name of the author or search books which cost less than price. Everything seems to be okay, but I am new to OOP so I would like to hear some advice on whether it is programmed a "good way" or not.

class Library(object):
    books = []
    written_by_author = []
    books_under_price = []

    def __init__(self, name, author, isbn, price):
        self.name = name
        self.author = author
        self.isbn = isbn
        self.price = price

    def addBook(self):
        Library.books.append((self.name,self.author,self.isbn,self.price))

    def searchBookISBN(self,isbn):
        for book in Library.books:
            if book[2]==isbn:
                return book

    def searchBookAuthor(self,author):
        for book in Library.books:
            if book[1]==author:
                Library.written_by_author.append(book)
        return Library.written_by_author

    def searchUnderPrice(self,price):
        for book in Library.books:
            if book[3]<price:
                Library.books_under_price.append(book)
        return Library.books_under_price


book = Library('Geometry', 'Jeff Potter', '0596805888', 22)
book.addBook()
book = Library('Math', 'George Harr', '0594805888', 15)
book.addBook()
book = Library('English', 'James Odd', '0596225888', 10)
book.addBook()
book = Library('Physics','Jeff Potter','0597884512',18)
print (book.searchBookISBN('0594805888'))
print (book.searchBookAuthor('George Harr'))
print (book.searchUnderPrice(20))
\$\endgroup\$

migrated from stackoverflow.com Nov 24 '17 at 10:07

This question came from our site for professional and enthusiast programmers.

26
\$\begingroup\$
  • Your design doesn't follow best practice. Your Library class should create a library when you use __init__, rather than a book. And so I'd split your code in half, create two classes; one for books, the other for the library.
  • You shouldn't mutate data on Library. This is as it mutates it on the class, rather than on the instance. And so if you make a new instance, then it will be tied to all the other instances.
  • When making the book class. This can significantly be simplified with collections.namedtuple. Such as:

    Book = namedtuple('Book', 'name author ISBN price')
    

And so I'd change your code to:

from collections import namedtuple


class Library(object):
    def __init__(self):
        self.books = []

    def addBook(self, book):
        self.books.append(book)

    def searchBookISBN(self, ISBN):
        for book in self.books:
            if book.ISBN == ISBN:
                return book

    def searchBookAuthor(self, author):
        written_by_author = []
        for book in self.books:
            if book.author == author:
                written_by_author.append(book)
        return written_by_author

    def searchUnderPrice(self, price):
        books_under_price = []
        for book in self.books:
            if book.price < price:
                books_under_price.append(book)
        return books_under_price


Book = namedtuple('Book', 'name author ISBN price')

library = Library()
library.addBook(Book('Geometry', 'Jeff Potter', '0596805888', 22))
library.addBook(Book('Math', 'George Harr', '0594805888', 15))
library.addBook(Book('English', 'James Odd', '0596225888', 10))
library.addBook(Book('Physics', 'Jeff Potter', '0597884512', 18))
print(library.searchBookISBN('0594805888'))
print(library.searchBookAuthor('George Harr'))
print(library.searchUnderPrice(20))

After this, I'd recommend:

Which would further improve your code to:

class Library(object):
    def __init__(self):
        self.books = []

    def add_book(self, book):
        self.books.append(book)

    def book_with_ISBN(self, ISBN):
        for book in self.books:
            if book.ISBN == ISBN:
                return book

    def books_by_author(self, author):
        return [book for book in self.books if book.author == author]

    def books_under_price(self, price):
        return [book for book in self.books if book.price < price]
\$\endgroup\$
  • 2
    \$\begingroup\$ Having O(1) lookup by keeping dictionaries around to search by ISBN or author might also be handy. \$\endgroup\$ – Graipher Nov 24 '17 at 13:11
  • \$\begingroup\$ @Graipher yes you could, feel free to make that an answer. 🙂 I feel that as we don't know how the code is used that premature optimisations could be a bad idea \$\endgroup\$ – Peilonrayz Nov 24 '17 at 13:21
  • 1
    \$\begingroup\$ Done. And I agree about premature optimizations, but in general I would expect a library to have more than three books :) \$\endgroup\$ – Graipher Nov 24 '17 at 13:55
  • \$\begingroup\$ @Graipher I would too, however this reads like a homework question. Which only has like four books. So IMO the maintenance cost here would outweigh the speed benefit. Otherwise I'd fully agree, +1 to your answer too. \$\endgroup\$ – Peilonrayz Nov 24 '17 at 14:18
  • \$\begingroup\$ You could also make books_by_author and books_under_price generator functions or return a filter(). \$\endgroup\$ – Richard Neumann Nov 24 '17 at 14:29
12
\$\begingroup\$

This is an addendum to Peilonrayz' excellent answer.

Depending on how large your library will grow, searching linearly through the books to find a specific author or ISBN (or even books below a price), can take a long time (it grows \$\mathcal{O}(n)\$, obviously).

So you could keep a dictionary for the ISBNs and the authors around to quickly find the right book:

from collections import defaultdict

class Library(object):
    def __init__(self):
        self.books = []
        self.index_from_author = defaultdict(list)
        self.index_from_isbn = {}

    def add_book(self, book):
        self.books.append(book)
        index = len(self.books) - 1
        self.index_from_author[book.author].append(index)
        self.index_from_isbn[book.ISBN] = index

    def book_with_ISBN(self, ISBN):
        return self.books[self.index_from_isbn[ISBN]]

    def books_by_author(self, author):
        return [self.books[i] for i in self.index_from_author[author]]

    ...

This assumes that there is only exactly one book with each ISBN (not an unreasonable assumption), but that one author can have multiple books (also reasonable).

Note that if you want to remove a book from the library, that code would also need to handle removing it from these two dictionaries.

The price below some threshold is a bit trickier. You can either live with the linear time or you could keep the books in a heap, which is sorted by price. Then you would insert the book at the right position and could do a binary search for all books below some price. But this is a bit more involved and I don't have the time to write an example solution for that, right now.

\$\endgroup\$
6
\$\begingroup\$

There is problem with implementation because of lack of imagination in Object Oriented style. With more experience you will have better ideas. ;)

OOP is mainly about objects. What you created looks like your Libraries are Books. I do not think Book is extending Library. Instead (from my point of view) Library contains many Books.

As suggested the application could consist of two classes. The Library and the Book.

Lets create this application in iterations.

First iteration gives you objects

  • create class Book containing name, author, ISBN, price
  • create class Library containing books collection (set, list) or array

Now what you have - you can get book (if you know which book you want) from the Library.

Second iteration brings new methods

Wanted methods are add books, search for ISBN and search by authors name or finding lower priced books. Well these could be other functions on top of different collections.

As my preferred language is Java I will just give suggestions:

  • Add books should be on top of the books collection in Library
  • search for ISBN would be well crafted for Map (ISBN, Book)
  • search for authors name would be well crafted for MultiMap (author name, Book)
    • Parameters: First name and Last name, maybe even year when born..
  • finding books with lower price would be nicely done with Map (price, Book)

Third iteration would be for optimizations

Well basically to find better ideas for implementations. It would not be necessary to add Book to 3 different collections. Instead it could be nice to add Book to one collection and query it from different.

Example:

HashMap<ISBN, Book> booksMap
HashMultiMap<Author, ISBN> authorsBooks - like HashMap<Author, List<ISBN>>
TreeMap<Price, ISBN> booksByPrice

and after finding ISBNs use it on bookMap (to get the Book and not just ISBN).

Reasons why I used those Maps:

HashMap is O(1) (aka. instant) while having good hash function and size.

TreeMap has sorted keys with reasonable O(log(n)) and therefore just iterating over books until price < maxPrice could be fine.

There could be reasons why to choose different collections, but just to display the last step I chose the simplest case.

\$\endgroup\$
1
\$\begingroup\$

Personally I would abstract it more and start with a book class which has all the information about that specific book and then just have a library class which brings all the books together and deals with them. Maybe even have a user class which holds the information for each user

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.