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))
4 Answers 4
- 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:
- You read and follow PEP 8, so that your code is easier to read.
- You learn list comprehensions
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]
-
2\$\begingroup\$ Having O(1) lookup by keeping dictionaries around to search by ISBN or author might also be handy. \$\endgroup\$Graipher– Graipher2017εΉ΄11ζ24ζ₯ 13:11:45 +00:00Commented Nov 24, 2017 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\$2017εΉ΄11ζ24ζ₯ 13:21:29 +00:00Commented Nov 24, 2017 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– Graipher2017εΉ΄11ζ24ζ₯ 13:55:14 +00:00Commented Nov 24, 2017 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\$2017εΉ΄11ζ24ζ₯ 14:18:52 +00:00Commented Nov 24, 2017 at 14:18
-
\$\begingroup\$ You could also make
books_by_author
andbooks_under_price
generator functions or return afilter()
. \$\endgroup\$Richard Neumann– Richard Neumann2017εΉ΄11ζ24ζ₯ 14:29:03 +00:00Commented Nov 24, 2017 at 14:29
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.
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 Librarysearch 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.
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