Inserting and displaying books

Question

I'm trying to write a program where you can insert and display some books (without using a database).

For doing this, I use three classes:

• Book - is the base class.
• TehnicBook and Literature - that will inherit some properties from Book.

#include <iostream>
#include <string>
#include <stdlib.h>
using namespace std;
class Book {
public:
 string author, title;
 bool rented;
 Book(string &author, string &title, bool rented) {
 this -> author = author;
 this -> title = title;
 this -> rented = rented;
 };
 void display(void);
};
class TehnicBook:public Book {
 int amount, RlYear;
 string language;
 TehnicBook *head, *next;
public:
 TehnicBook(string &author, string &title, bool rented, int amount, string &language, int RlYear):Book(author, title, rented) {
 head = NULL;
 this -> language = language;
 this -> amount = amount;
 this -> RlYear = RlYear;
 };
 ~TehnicBook(void) {
 delete head;
 };
 void display(void);
 void add(void);
 void dellete(string&);
};
class Literature:public Book {
 string bookType;
 Literature *head, *next;
public:
 Literature(string &author, string &title, bool rented, string &bookType):Book(author, title, rented) {
 head = NULL;
 this -> bookType = bookType;
 };
 ~Literature(void) {
 delete head;
 };
 void display(void);
 void add(void);
};
void TehnicBook::add(void) {
 string author, title, language;
 int year, amount;
 bool rented;
 cout << endl << "Author: ", cin >> author;
 cout << "Title: ", cin >> title;
 cout << "Rented? (0/1): ", cin >> rented;
 cout << "The amount of books: ", cin >> amount;
 cout << "Language: ", cin >> language;
 cout << "Release year: ", cin >> year;
 TehnicBook *p = new TehnicBook(author, title, rented, amount, language, year);
 p -> next = head;
 head = p;
}
void TehnicBook::display(void) {
 TehnicBook *p = head;
 while(p) {
 cout << "-----------------------------\n";
 cout << "Author: " << p -> author << endl;
 cout << "Title: " << p -> title << endl;
 cout << "Is " << ((p -> rented) ? "" : "not ") << "rented" << endl;
 cout << "Amount of books: " << p -> amount << endl;
 cout << "Language: " << p -> language << endl;
 cout << "Release year: " << p -> RlYear << endl;
 cout << endl;
 p = p -> next;
 }
}
void Literature::add(void) {
 string author, title, bookType;
 bool rented;
 cout << "\nAuthor: ", cin >> author;
 cout << "Title: ", cin >> title;
 cout << "Is rented? ", cin >> rented;
 cout << "Book type (hardcover/...: ", cin >> bookType;
 Literature *p = new Literature(author, title, rented, bookType);
 p -> next = head;
 head = p;
}
void Literature::display(void) {
 Literature *p = head;
 while(p) {
 cout << "\n-----------------------------\n";
 cout << "Author: " << p -> author << endl;
 cout << "Title: " << p -> title << endl;
 cout << "Is rented? " << ((p -> rented) ? "yes" : "no") << endl;
 cout << "Book type: " << p -> bookType << endl << endl;
 p = p -> next;
 }
}
int main(int argc, char const **argv) {
 string blank = "";
 TehnicBook *tehnicB = new TehnicBook(blank, blank, false, 0, blank, 0);
 Literature *litB = new Literature(blank, blank, false, blank);
 int opt;
 for(;;) {
 cout << "\n\n1) Add a tehnic book.\n";
 cout << "2) Display all tehnic books.\n";
 cout << "3) Add a literature book.\n";
 cout << "4) Display all literature books.\n";
 cout << "5) Exit.\n\n";
 cout << "Your option: ", cin >> opt;
 switch(opt) {
 case 1:
 tehnicB -> add();
 break;
 case 2:
 tehnicB -> display();
 break;
 case 3:
 litB -> add();
 break;
 case 4:
 litB -> display();
 break;
 case 5:
 exit(0);
 default:
 continue;
 }
 }
 return 0;
}

Am I doing this right? Any better ideas?

• Edward: It doesn't seem to actually work. When I put in "Marius Marusanici" as the author for the first book, the code simply infinitely repeats the menu.
• mariusmmg2: @Edward Yep, it only works when you insert something without spaces. I think this is how cin works with strings.

Answer 1

• Major bug: Book::~Book is not virtual. This means you have a memory leak in the following case:

  TehnicBook *tb = new TehnicBook(...);
  tb->add(...);
  Book *b = tb;
  delete tb; //<- tb->head is leaked
  

  If you have C++11 I recommend that every class either has a virtual destructor or is declared final such as class Book final{...}.

• Major design issue: You are combining TehnicBook with a linked list. A class should have only one responsibility: Either manage a book or manage memory, not both. At least use std::list or better std::vector. Better remove add from TehnicBook and Literature and put a vector<Book> into main to seperate storage and functionality.

• Similarly add should not use cin and cout. You are mixing user interface and class functionality. It should simply take a TehnicBook *. You can make a free standing function that conveniently does this, but it should not be part of TehnicBook.

• Similarly display should not use cout. Either make it return a string which you then give to cout or teach cout how to print books such as this:

  ostream &operator <<(ostream &os, TehnicBook &t){
   os << 
   os << "-----------------------------\n";
   os << "Author: " << p->author << endl;
   os << "Title: " << p->title << endl;
   os << "Is " << ((p->rented) ? "" : "not ") << "rented" << endl;
   os << "Amount of books: " << p->amount << endl;
   os << "Language: " << p->language << endl;
   os << "Release year: " << p->RlYear << endl;
   os << endl;
   return os;
  }
  

  Now you can do TehnicBook book(...); cout << book;, which is pretty neat, but more importantly you can do things like ofstream file("test.txt"); file << book;, so we got printing a book to a file and TCP-streams and so on for free.

• I see void dellete(string&); inside TehnicBook, but no implementation. I guess you meant to add the functionality to remove books from the list. Remove this function, it does not belong to a TehnicBook.

• Is TehnicBook supposed to be TechnicBook or TechnicalBook?

• This may be a bit over your head, but I want to at least mention it: Do not use new and delete. Ever. Instead use make_unique.

  TehnicBook *tehnicB = new TehnicBook(blank, blank, false, 0, blank, 0);
  

  becomes

  auto tehnicB = make_unique<TehnicBook>(blank, blank, false, 0, blank, 0);
  

  The point is that now you do not need to manage memory. Managing memory is very difficult and error prone and unnecessary. For example you forgot to clean up tehnicB and litB inside main. This automatically gets fixed without thinking about it with modern C++.

• Edward: A book collection could be std::vector<std::unique_ptr<Book>> library; and then books could be added as library.emplace_back(std::make_unique<Literature>("Tolstoy", "War and Peace", false, "hardcover"));
• Edward: You might also want to mention how to actually implement polymorphism. For instance, define virtual std::ostream &display(std::ostream &out) in Book and then implement display in derived classes, calling Book::display first. Then use it as std::ostream &operator<<(std::ostream &out, const Book& b) { return b.display(out); }
• nwp: @Edward Unless it is a specific question I'd rather recommend a book.
• Loki Astari: A class should have only one responsibility: Pattern called "Separation of Concerns"
• Loki Astari: The point is that now you do not need to manage memory. Correct. So don't even dynamically allocate when automatic variables will do.

Answer 2

Bug: you cannot enter multiple words for any of the string arguments, for example author and title. It will make more sense to read full lines there, for example:

cout << endl << "Author: ";
getline(cin, author);

Please don't use using namespace std (read this). This is ok though:

using std::cin;
using std::cout;
using std::endl;
using std::string;

But the best is to not do this at all, but use explicitly std::cout when you need, especially for common names like string, which might easily conflict with other implementations. See also @Loki's comment.

Instead of #include <stdlib.h>, write like this: #include <cstdlib>. But actually, as @Edward pointed out, it would be better to remove this import, and change the single exit(0) statement to return 0.

It's unusual to put spaces around ->. This is more common:

this->author = author;

RlYear is not a very good name. Use camelCase for variable names in general. For this variable, releaseYear would be a better name.

You can simplify the constructor of Book like this:

Book(string &author, string &title, bool rented) :
 author(author), title(title), rented(rented) {}

Similarly, TechnicBook like this:

TehnicBook(string &author, string &title, bool rented, int amount, string &language, int RlYear) :
 Book(author, title, rented), head(NULL), language(language), amount(amount), RlYear(RlYear) {}

Lastly, although the title says "c++ polymorphism", there's no example of polymorphism in this code.

• janos: @Edward g++ cannot find exit without that line
• Edward: True, but then that line would be better as return 0; anyway.
• janos: @Edward good call, updated my answer, thanks for that!
• Loki Astari: This is ok though: Depends: In global scope it is better (not ideal (especially string it is way too common an identifier to be put in the global namespace)). Best is to just be specific std::cout << "Works fine\n";
• janos: Thanks @LokiAstari, I added a comment on that to clarify.