HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Inserting and displaying books

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
insertingandbooksdisplaying

Problem

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
#include
#include

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 > author;
cout > title;
cout > rented;
cout > amount;
cout > language;
cout > 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 author title rented) ? "" : "not ") amount language RlYear next;
}
}

void Literature::add(void) {
string author, title, bookType;

Solution

-
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; //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 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 author title rented) ? "" : "not ") amount language RlYear << endl;
    os << endl;
    return os;
}


Now you can do TehnicBook book(...); cout

-
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(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++.

Code Snippets

TehnicBook *tb = new TehnicBook(...);
tb->add(...);
Book *b = tb;
delete tb; //<- tb->head is leaked
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;
}
TehnicBook *tehnicB = new TehnicBook(blank, blank, false, 0, blank, 0);
auto tehnicB = make_unique<TehnicBook>(blank, blank, false, 0, blank, 0);

Context

StackExchange Code Review Q#68219, answer score: 3

Revisions (0)

No revisions yet.