11
\$\begingroup\$

Very new to OOP with Python - and feel like I still don't totally get OOP. However I thought I'd put together a personal online book library to practice inheritance, methods, docstrings etc.

Program allows one to add books (comics or novels - with comics inheriting from novels) to one's own library, and create a custom reading lists.

There are a few different methods. For example, a library method which will return all books over a specified length.

Note that I've removed quite a lot from this, just so it's not 5 pages long! I would normally have several different types of books, attributes such as Author etc. But hopefully from the below you get a good idea of the structure.

Speaking of structure, I would just like to know if mine is any good. Also there is an issue whereby if you remove a book from the library, it will still show up in any reading lists I created. I don't know if there's a way to deal with this, or if I'm thinking about this the wrong way.

class Library: def __init__(self, books = None): if books is None: self.books = [] else: self.books = books def add_book(self, book): self.books.append(book) def remove_book(self, book): self.books.remove(book) def pages_over_n(self, number): """Returns books that have more than specified number of pages""" return [book for book in self.books if book.pages > number] def get_library(self): """Returns all books in Library""" return self.books def __repr__(self): return f"Library({self.books})" def __str__(self): return f"Library has {len(self.books)}" class Novel: def __init__(self, title, pages, publicationDate): self.title = title self.pages = pages self.publicationDate = publicationDate def __repr__(self): return f"Novel({self.title}, {self.pages}, {self.publicationDate})" def __str__(self): return f"Media: {self.title}" class ComicBook(Novel): def __init__(self, title, pages, publicationDate, artist): super().__init__(title, pages, publicationDate) self.artist = artist def get_artist(self): return f"Artist: {self.artist}" def __repr__(self): return (f"ComicBook({self.title}, {self.pages}, " f"{self.publicationDate}, {self.artist})") class ReadingList: def __init__(self): self.list = [] def add_book(self, book): """Add book to reading list""" self.list.append(book) def remove_book(self, book): """Remove book from reading list""" self.list.remove(book) def total_pages(self): """Returns total pages in reading list""" total = 0 for book in self.list: total += book.pages return total def __repr__(self): return f"ReadingList({self.list})" def __str__(self): return f"Reading list of size {len(self.list)}" # Initialise Library library = Library() # Create a few books novel1 = Novel("Harry Potter", 500, 1991) novel2 = Novel("LotR", 1000, 1960) novel3 = Novel("Game of Thrones", 2000, 2018) comic1 = ComicBook("Batman", 100, 2020, "Stan Lee") # Add books to library. library.add_book(novel1) library.add_book(novel2) library.add_book(novel3) library.add_book(comic1) # Create a new reading list. readingListOne = ReadingList() # Add a couple of books to reading list. readingListOne.add_book(novel1) readingListOne.add_book(comic1)
\$\endgroup\$
8
  • 8
    \$\begingroup\$ Long code is not frowned upon in Code Review. For a big-ish project, you can also put the code on git(hub/lab) etc and include the specific file/code you want reviewed in the post here, linking to the project for further reference. \$\endgroup\$ Commented Oct 9, 2020 at 10:07
  • \$\begingroup\$ Thanks for the info. I had the feeling that people would only be interested in looking over short code. There isn't too much more to add at the moment (still working through it) - jus wanted to check I was on the right lines. \$\endgroup\$ Commented Oct 9, 2020 at 10:10
  • \$\begingroup\$ @AaronWright Do you have to do OOP? A lot of people (including myself) don't like it, and would prefer to do a functional style. \$\endgroup\$ Commented Oct 10, 2020 at 3:32
  • \$\begingroup\$ There is no persistence of data yet. \$\endgroup\$ Commented Oct 10, 2020 at 7:09
  • 1
    \$\begingroup\$ @dwjohnston You don't like OOP and your top tag is java? \$\endgroup\$ Commented Oct 10, 2020 at 9:55

3 Answers 3

Reset to default
8
\$\begingroup\$

General Observations

I'm being 100% honest here, When I just give a glance at your code there is nothing that is immediately horrifying with it.

  • You shouldn't use variable/function names that are already defined in Python. Your class ReadingList has an attribute list. This is a bad idea as there is already a list keyword that can clash with your definition

Code Structure

You have tried to implement the idea of inheritance, although it is fine, it will be much better to have your base class as Book. b

This makes more sense due to the fact that novels, and comic books are types of books. And hence can share attributes and methods.

Your Book class can have attributes and methods like

  • Author
  • Name
  • Stuff which is common in all types of books

And your child class Novel can have

  • Sutff which is special in Novel

A very very basic implementation

class Book(): def __init__(self,author,name): self.author = author self.name = name def print_book(self): print(f"{self.name} by {self.author}") class Novel(Book): def __init__(self,author,name): Book.__init__(self,author,name) def display(self): print(f"The Novel {self.name}") # Novel stuff which isn't common in all books class Comic(Book): def __init__(self,author,name): Book.__init__(self,author,name) def display(self): print(f"The comic book {self.name}") # Comic book stuff which isn't common in all books nov = Novel("J.K Rowling","Harry Potter") nov.print_book()

The point is that your Novel class is basically a Book which as an author, pages, etc. But it also has something which other books like Comic don't. This way you don't need to create an entirely new class, but you can simply inherit the common from the Book class and add the extra stuff in Novel

Creating a library

You can stop here if you want to, and consider your Book class as a library. But if you want to get crazier, you can have another class called Magazine and that would be the child class of parent Library. Here is what that would look like. lib

But this would get quite complicated, moreover, it wouldn't make a lot of sense to inherit Book from Library. Hence it is a good idea to keep Library as a separate that will merely control the books.

 class Library(): def __init__(self,book_list = []): self.book_list = book_list def new_book(self,book): self.book_list.append(book) def remove_book(self,name): # book is the name of the book for book in self.book_list: if book.name == name: self.book_list.remove(book) break def print_books(self): for book in self.book_list: print(f"Book {book.name} by {book.author}") class Book(): def __init__(self,author,name): self.author = author self.name = name def print_book(self): print(f"{self.name} by {self.author}") class Novel(Book): def __init__(self,author,name): Book.__init__(self,author,name) def display(self): print(f"The Novel {self.name}") # Novel stuff which isn't common in all books class Comic(Book): def __init__(self,author,name): Book.__init__(self,author,name) def display(self): print(f"The comic book {self.name}") # Comic book stuff which isn't common in all books my_lib = Library() my_lib.new_book(Novel("J.K Rowling","Harry Potter")) my_lib.new_book(Novel("Aryan Parekh","Stack")) my_lib.remove_book("Stack") my_lib.print_books()
\$\endgroup\$
6
  • \$\begingroup\$ Thank you. Yes having Book as the parent class would make more sense now I think about it. I'll be sure to change list as well. Regarding having magazine and book as child classes of library, I'm not quite sure how this would work. However this is probably just down to me not being that knowledgable regarding inheritance. What kind of methods would the parent class Library have in this instance? \$\endgroup\$ Commented Oct 9, 2020 at 11:23
  • 1
    \$\begingroup\$ @AaronWright It is not absolutely necessary that it would need to be a parent. In fact, I would recommend Library to be a separate class which would create a list of Book objects \$\endgroup\$ Commented Oct 9, 2020 at 13:01
  • \$\begingroup\$ Ah I see. Thank you. I think this is similar to what I have, but just having subclasses of books which inherit from Book. \$\endgroup\$ Commented Oct 9, 2020 at 13:22
  • \$\begingroup\$ @AaronWright Inheritance is useful, but if you ask me, I would say no to use it in this scenario \$\endgroup\$ Commented Oct 9, 2020 at 13:24
  • 1
    \$\begingroup\$ Small suggestion/nitpick: it is common to just use books instead of book_list \$\endgroup\$ Commented Oct 10, 2020 at 8:21
8
\$\begingroup\$

Simplify the constructor of class Library

You have a default value of None for the parameter books, but then immediately proceed to convert that None to an empty list. Why not use [] as the default value?

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

Note that to prevent the issue of mutable default arguments as mentioned by ojdo, you should make a copy of the initial list.

Be consistent

You take an optional list of books in the constructor of class Library, but there is no such thing in class ReadingList. That is a bit surprising.

Also be consistent with naming things. In class Library, the list of books is called books, but in class ReadlingList you named it list. I would name it books in both cases.

Also be consistent with the names of functions:

  • get_library() should be get_books(), since it only returns the list of books, not an object of type Library.
  • pages_over_n() should be get_books_with_over_n_pages() or something similar, to indicate that it returns Books, not pages.
  • total_pages() should be renamed get_total_pages() or calculate_total_pages(). Prefer using verbs for function names, and nouns for variables.

And I also wonder why a ComicBook has an artist, but a Novel has no author? And why is there a get_artist() but no get_title(), get_publication_date() and so on? Making everything consistent will make using your classes much easier.

Removing books from the Library

As you noticed, you can remove a book from the library, but it won't be removed from any reading lists. But that is fine. Why would a book magically disappear from a real life reading list if your library removes that book? Note that your code is perfectly fine, Python won't actually delete the Book object from memory until the last reference to it is gone.

However, if you want a book to disappear from the ReadingList when it is removed from the Library, then somehow there needs to be some communications between those two objects. There are two options:

  1. A Library keeps a list of ReadingLists that reference its Books, and when you call remove_book() on a Library, it will in turn iterate over all the ReadingLists and call remove_book() on them as well.

  2. A ReadingList keeps a reference to the Library whose Books it contains. Whenever you call a member function that accesses the list of books on the reading list, this member function first has to filter out all the books that are no longer in the Library.

\$\endgroup\$
7
  • \$\begingroup\$ Perfect thank you! This helps a lot. Your first point makes a lot more sense than what I've done. Regarding the lack of get methods, this is something I plan on adding in. For the reading list, my thinking was that if you remove a book from the Library, then you would also want it removed from the reading list. Sort of like removing a song from Spotify's library would remove it from a playlists (I would presume). Again, this is just a vague idea I have. \$\endgroup\$ Commented Oct 9, 2020 at 11:16
  • 7
    \$\begingroup\$ I fear the first recommendation would backfire, or do you know that this behaves differently within instance methods? \$\endgroup\$ Commented Oct 9, 2020 at 11:49
  • 1
    \$\begingroup\$ @ojdo You are right. But actually, even when you provide the argument you have to be aware that the list you provide might be modified by subsequent calls to add_book() and remove_book(), so I guess it's best to always create a copy of the list. I'll modify my answer. \$\endgroup\$ Commented Oct 9, 2020 at 12:25
  • 1
    \$\begingroup\$ @AaronWright This has not much to do with OOP, but rather how variables work in Python. \$\endgroup\$ Commented Oct 9, 2020 at 13:38
  • 4
    \$\begingroup\$ I personally would a) not opt to do perform a copy operation and b) prefer the books=None idiom like in the question, but in total a helpful answer, +1. \$\endgroup\$ Commented Oct 9, 2020 at 17:23
6
\$\begingroup\$

Since Library and ReadingList are just different types of lists of books, I would opt for define a generic BookList as a base class from which Library and ReadingList both inherit.

Doing this saves you from the repetition of having to define common methods like the following, which both of your classes already do. Here are some methods from the Library class.

def __init__(self, books = None): if books is None: self.books = [] else: self.books = books def add_book(self, book): self.books.append(book) def remove_book(self, book): self.books.remove(book)

And here are the analogous methods from the ReadingList class.

def __init__(self): self.list = [] def add_book(self, book): """Add book to reading list""" self.list.append(book) def remove_book(self, book): """Remove book from reading list""" self.list.remove(book)

As G. Sliepen said, you should simplify the Library constructor to match the ReadingList constructor. After you do this, the methods are exactly the same.

By defining a generic book list class, not only do you not have to keep redefining common functionality, you can also take advantage of polymorphic functions.

For example, rather than making each class responsible for outputting a string representation of itself, you could define a BookListHTMLWriter that takes a book list item (either a ReadingList or a Library, both are fine when they're derived from the same BookList object), and outputs an HTML table representation.

Likewise, you could also define a BookListXMLWriter that outputs the XML representation of a BookList object or a subclass thereof. You could take this OOP-refactoring even further by then refactoring the functionality common to the BookListHTMLWriter and BookListXMLWriter classes and derive them from a base BookListWriter base class.

Optimistically, object-oriented programming can be a bit of an acquired taste (and an esoteric one, at that), but once you start thinking in these terms it's a useful mental model, even if it's not your only one. Or you preferred one, even.

Regarding your point about how removing a book from your library does not remove it from your reading list(s), I agree with previous responses that this makes sense. I think the problem that you're alluding to might be the question of referential integrity, which I noticed, too.

You've obviously already noticed that Library and ReadingList are both lists of Books. When reading over code, I always start picturing the corresponding database schema, and the problem I think you're referring to is that there should be a main list of already created books from which you can add to a personal library and/or reading list, presumably depending on whether you own the book. I would call this main list a "catalog" or something to differentiate this main list from a list of a particular type belonging to a particular user.

Moreover, this catalog would simply be all of the records in a books table in your database.

To implement a database for this program, you could start by creating the books and users tables, and then creating a third table consisting only of a user's id and a book's id. Querying this link table for a given user id would give you all of the books this person owns.

If you wanted users to be able to create arbitrary libraries based not just on whether they own a book, you would only need to create a reading_lists table with a user id as a field to indicate the owner, and then create another link table using a list id and a book id.

This would solve your referential integrity problem by preventing the deletion of a book record unless you specify an appropriate ON DELETE action in the books table schema.

Subclassing Books into Novels and Magazines complicates things because if the inheritance hierarchy imbues complex enough differences on your subclasses, your objects no longer map nicely into single tables (assuming you want a reasonably-normalizable schema).

If you hit this point, a good entity-relational mapping tool can save you hours of frustration.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you! I'll give some of this a go. Some of it may be a little advanced for me, however sounds like a good learning opportunity. Even defining BookListXMLWriter sounds like something I'll need to do a bit of research into. \$\endgroup\$ Commented Oct 9, 2020 at 13:10
  • \$\begingroup\$ Do you know where I could look to learn a bit more about implementing databases etc, or a link to something that has done something similar? It's a bit beyond my knowledge, but sounds interesting. \$\endgroup\$ Commented Oct 9, 2020 at 13:58
  • \$\begingroup\$ If you've never worked with databases before, I can't recommend Beginning Databases with PostgreSQL enough. It was published years ago, but the content is still current. It walks you through the setup, designing a database, why even bother, etc. It's one of my favorite books of all time, I love it. (It's also only 11 bucks used) It's the book that got me started \$\endgroup\$ Commented Oct 9, 2020 at 18:58
  • \$\begingroup\$ Perfect thank you! I'll definitely take a look. I'm doing a course which includes databases and SQL - which I think starts in a couple months, but I'll try get a head start with that book! \$\endgroup\$ Commented Oct 9, 2020 at 20:50

You must log in to answer this question.