c style pointers in c++

[bluefrog]

I am attempting to use c style pointers in c++.
I've created a Book class (base) and BookCategory (derived) class.
All pointers are initialised to nullptr.
All deletes are coded as "delete []".
A segment fault runtime error is however generated.

Could anybody suggest what I am doing incorrect?

#include <iostream>
#include <cstring>

class Book {
  private:
    char * authors{nullptr};
    char * title{nullptr};
    int sold;
    double price;

  public:
    Book(char *, char *, int, double);
    Book(const Book &);
    Book();
    virtual ~Book();
    virtual void Report() const; // report
    Book & operator=(const Book &);
};

Book::Book(char * s1, char * s2, int n, double x) : authors{s1}, title{s2}, sold{n}, price{x} { }
Book::Book() : authors{nullptr}, title{nullptr}, sold{0}, price{0.0} { };

Book::~Book() {
  delete [] authors;
  delete [] title;
};

Book::Book(const Book & b) {
  authors = new char[std::strlen(b.authors) + 1];
  std::strcpy(authors, b.authors);
  title = new char[std::strlen(b.title) + 1];
  std::strcpy(title, b.title);
  sold = b.sold;
  price = b.price;  
}

Book & Book::operator=(const Book & b) {
  if (this == &b)
    return *this;

  delete [] authors;
  delete [] title;
  authors = new char[std::strlen(b.authors) + 1];
  std::strcpy(authors, b.authors);
  title = new char[std::strlen(b.title) + 1];
  std::strcpy(title, b.title);

  sold = b.sold;
  price = b.price;
  return *this;
}

void Book::Report() const {
  std::cout << "Author " << authors << " Title " << title ;
};

class BookCategory : public Book {
  private:
    char * category{nullptr};
  public:
    BookCategory(char *, char *, char *, int, double);
    BookCategory(const BookCategory &);
    BookCategory();
    virtual ~BookCategory();
    virtual void Report() const; // report
    BookCategory & operator=(const BookCategory &);
};

BookCategory::BookCategory(char * s1, char * s2, char * bc, int n, double x) :
  Book{s1, s2, n, x}, category{bc} { }

BookCategory::BookCategory() : Book{}, category{nullptr} { };

BookCategory::~BookCategory() {
  delete [] category;
};

BookCategory & BookCategory::operator=(const BookCategory & bc) {
  if (this == &bc)
    return *this;
  Book::operator =(bc);
  delete [] category;
  category = new char[std::strlen(bc.category) + 1];
  std::strcpy(category, bc.category);
  return *this;
}

BookCategory::BookCategory(const BookCategory & bc) : Book(bc) {
  category = new char[std::strlen(bc.category) + 1];
  std::strcpy(category, bc.category);
}

void BookCategory::Report() const {
  Book::Report();
  std::cout << " Category " << category << "\n";
};

void BookReport(const Book & b);

int main() {
  char * a = (char*)"Bjarne Stroustrup";
  char * b = (char*)"The C++ Programming Language 4th Ed";
  Book b1(a, b, 14, 35.5);
  return 0;
}

void BookReport(const Book & b) {
  b.Report();
}

[Peter87]

First of all I don't think you should cast string literals to non-const char*. They are const for a reason. Instead you should use const char* for a and b in main and also for the function arguments.

I think the real problem is that you do not copy the strings passed to the Book(const char*, const char*, int, double) constructor, so when the destructor later tries to free them using delete[] it fails. Only things created with new should be freed with delete.

[bluefrog]

thanks for the help!

I didn't realise the constructors had to allocate memory. Seems to be fine now.
Can you suggest any other improvement, apart from using smart pointers?

#include <iostream>
#include <cstring>

class Book {
  private:
    char * authors{nullptr};
    char * title{nullptr};
    int sold;
    double price;

  public:
    Book(const char *, const char *, int, double);
    Book(const Book &);
    Book();
    virtual ~Book();
    virtual void Report() const; // report
    Book & operator=(const Book &);
};

Book::Book(const char * s1, const char * s2, int n, double x) : sold{n}, price{x} {
  authors = new char[std::strlen(s1) + 1];
  std::strcpy(authors, s1);
  title = new char[std::strlen(s2) + 1];
  std::strcpy(title, s2);
}

Book::Book() : authors{nullptr}, title{nullptr}, sold{0}, price{0.0} { };

Book::~Book() {
  delete [] authors;
  delete [] title;
};

Book::Book(const Book & b) {
  authors = new char[std::strlen(b.authors) + 1];
  std::strcpy(authors, b.authors);
  title = new char[std::strlen(b.title) + 1];
  std::strcpy(title, b.title);
  sold = b.sold;
  price = b.price;  
}

Book & Book::operator=(const Book & b) {
  if (this == &b)
    return *this;

  delete [] authors;
  delete [] title;
  authors = new char[std::strlen(b.authors) + 1];
  std::strcpy(authors, b.authors);
  title = new char[std::strlen(b.title) + 1];
  std::strcpy(title, b.title);

  sold = b.sold;
  price = b.price;
  return *this;
}

void Book::Report() const {
  std::cout << "Author " << authors << " Title " << title ;
};

class BookCategory : public Book {
  private:
    char * category{nullptr};
  public:
    BookCategory(const char *, const char *, const char *, int, double);
    BookCategory(const BookCategory &);
    BookCategory();
    virtual ~BookCategory();
    virtual void Report() const; // report
    BookCategory & operator=(const BookCategory &);
};

BookCategory::BookCategory(const char * s1, const char * s2, const char * bc, int n, double x) : Book{s1, s2, n, x} {
  category = new char[std::strlen(bc) + 1];
  std::strcpy(category, bc);
}

BookCategory::BookCategory() : Book{}, category{nullptr} { };

BookCategory::~BookCategory() {
  delete [] category;
};

BookCategory & BookCategory::operator=(const BookCategory & bc) {
  if (this == &bc)
    return *this;
  Book::operator =(bc);
  delete [] category;
  category = new char[std::strlen(bc.category) + 1];
  std::strcpy(category, bc.category);
  return *this;
}

BookCategory::BookCategory(const BookCategory & bc) : Book(bc) {
  category = new char[std::strlen(bc.category) + 1];
  std::strcpy(category, bc.category);
}

void BookCategory::Report() const {
  Book::Report();
  std::cout << " Category " << category << "\n";
};

void BookReport(const Book & b);

int main() {
  const char * a = (char*)"Bjarne Stroustrup";
  const char * b = (char*)"The C++ Programming Language 4th Ed";
  Book b1(a, b, 14, 35.5);
  BookReport(b1);
  return 0;
}

void BookReport(const Book & b) {
  b.Report();
}

[Peter87]

Can you suggest any other improvement, apart from using smart pointers?

Use std::string. ;-)

Inheritance describes an is-a relationship. To me it sounds a bit weird saying that a BookCategory is a Book. I think it would make more sense if a BookCategory was a class that contained a list of books, or if each book had a variable telling what category it belongs to.

[Thomas1965]

I would suggest that BookType and BookCategory should be an enum - like so:

enum class BookType
{
  Fiction,
  NonFiction
};

enum class BookCategory
{
  ScienceFiction,
  Drama
  // etc
};

http://reference.yourdictionary.com/books-literature/different-types-of-books.html

[bluefrog]

Hi Thomas,

thanks, I never realised one could create an enum class.
that might be helpfull indeed.