1

I'm trying to prepare a library management system for my school homework. I wrote the code but in bool Library::add_book(const Book&) function, I can not initialize book to a dynamic array. I overloaded = operator to initialize Book class and when I declare a Book object in main, it works. But for dynamic allocated array, it is not working.

#include <iostream>

class Author{
    public:
        std::string name;
        Author(){}
        Author(const std::string& name){
            this -> name = name;
        }
};

class Book{
    private:
        std::string _title, _isbn;
        int _edition;
        Author *_author = new Author;
    public:
        Book(){
            *_author = Author("NULL");
            _isbn = "INVALID";
        }
        Book(const std::string& title,
            const std::string& isbn,
            const std::string& author,
            int edition){
                _title = title;
                _isbn = isbn;
                _edition = edition;
                *_author = Author(author);
            }
        bool query_isbn(const std::string&);
        Book& operator=(const Book&);
        friend std::ostream &operator <<(std::ostream &, Book&);
};

class Library{
    private:
        Book *_book = nullptr;
        int *_numberOfBorrows = nullptr;
        int _numberOfBook, _maxNumberBook;
    public:
        Library(int maxNumberBook){
            _maxNumberBook = maxNumberBook;
            _book = new Book[_maxNumberBook];
            _numberOfBorrows = new int[_maxNumberBook];
            for(int i = 0; i < _maxNumberBook; i++){
                _book[i] = Book();
                _numberOfBorrows[i] = 0;
            }
        }
        ~Library(){
            delete []_book;
            _book = NULL;
            delete []_numberOfBorrows;
            _numberOfBorrows = NULL;
        }
        bool add_book(const Book&);
        void operator+= (const Book&);
        const Book& borrow_book(const std::string&);
        friend std::ostream& operator <<(std::ostream&, Library&);
};

bool Book::query_isbn(const std::string& isbn){
    if(_isbn == isbn)
        return true;
    else
        return false;
}

Book& Book::operator=(const Book& book){
    Book ret;
    ret._author -> name = book._author -> name;
    ret._edition = book._edition;
    ret._isbn = book._isbn;
    ret._title = book._title;
    return ret;
}

std::ostream &operator << (std::ostream& output, Book& obj){
    output << obj._author -> name << ", " << obj._title << ", " << obj._isbn << std::endl;
    return output;
}

bool Library::add_book(const Book& book){
    for(int i = 0; i < _maxNumberBook; i++){
        if(_book[i].query_isbn("INVALID")){
            _book[i] = book;
            _numberOfBook++;
            return true;
        }
    }
    return false;
}

void Library::operator+= (const Book& book){
    add_book(book);
}

const Book& Library::borrow_book(const std::string& isbn){
    for(int i = 0; i < _maxNumberBook; i++){
        if(_book[i].query_isbn(isbn)){
            _numberOfBorrows[i] = _numberOfBorrows[i] + 1;
            return _book[i];
        }
    }
    std::cout << "ISBN with " << isbn << " does not exist" << std::endl;
}

std::ostream& operator <<(std::ostream& output, Library& lib){
    for(int i = 0; i < lib._maxNumberBook; i++){
        output << lib._book[i];
        output << "Borrowed: " << lib._numberOfBorrows[i] << std::endl;
        for(int i = 0; i < 38; i++)
            output << '=';
            std::cout << std::endl;
    }
    return output;
}

int main(int argc, char** argv){
    Library lib(3);
    Book obj1("The 8051 Microcontroller & Embedded Systems", "1234-456789123", "Mazidi", 1),
        obj2("Fundamentals of Database Systems", "7899-456456123", "Elmasri", 2),
        obj3("Electric Circuits", "1478-258963258", "Nilsson", 3);
    lib.add_book(obj1);
    lib.add_book(obj2);
    lib.add_book(obj3);
    lib.borrow_book("6584-258963258");
    lib.borrow_book("1234-456789123");
    lib.borrow_book("7899-456456123");
    lib.borrow_book("1478-258963258");
    std::cout << lib;



    return 0;
}

I want it to output book properties, but it outputs same as below...

ISBN with 6584-258963258 does not exist
ISBN with 1234-456789123 does not exist
ISBN with 7899-456456123 does not exist
ISBN with 1478-258963258 does not exist
NULL, , INVALID
Borrowed: 0
======================================
NULL, , INVALID
Borrowed: 0
======================================
NULL, , INVALID
Borrowed: 0
======================================
Hanzla
  • 214
  • 5
  • 15
bacimen
  • 13
  • 3
  • 1
    You really should use your constructors initialization list, rather than the constructor body, whenever you can. – Jesper Juhl Dec 01 '19 at 17:20
  • 1
    Your `Book::operator=` is an elaborate no-op. It doesn't assign to `*this` - it assigns to a local variable, that is destroyed when the function returns. In other words, when you do, say, `book1 = book2;`, `book1` remains unchanged. You have an assignment operator that doesn't in fact assign. – Igor Tandetnik Dec 01 '19 at 18:42

1 Answers1

1

There are several mistakes playing with one another. I will only cover the two immediately relevant to the asked question.

Problem 1:

bool Library::add_book(const Book& book){
    for(int i = 0; i < _maxNumberBook; i++){
        if(_book[i].query_isbn("INVALID")){
            _book[i] = book;
            _numberOfBook++;
            return true;
        }
    }
    return false;
}

makes use of

Book& Book::operator=(const Book& book){
    Book ret;
    ret._author -> name = book._author -> name;
    ret._edition = book._edition;
    ret._isbn = book._isbn;
    ret._title = book._title;
    return ret;
}

Which does not assign to the given Book. Rather than assigning to *this, it creates a new Book and assigns to it. As a result, the assignments do nothing.

Side note: since Book ret; is a local variable, you cannot safely return a reference to it. It goes out of scope at the end of the function and the caller's left holding an invalid reference. Compiler probably warns you of this.

operator= should look like

Book& Book::operator=(const Book& book){
    *_author = *book._author;
    _edition = book._edition;
    _isbn = book._isbn;
    _title = book._title;
    return *this;
}

But it should not be necessary at all.

The reason a custom operator= is being used is to presumably handle the _author member, a pointer. This is a bad place to use an owned pointer. Either Book should not have ownership of the Author so that many books can share the same Author or _author should be automatically allocated so that many Books can have a copy of the same Author. Currently the copy approach is what's being used, but with a pointer to a dynamically allocated Author. Copy with dynamic allocation is not appropriate here.

Copy with an automatic variable or a non-owned pointer (or a std::shared_ptr) eliminates the need for a custom operator=, eliminating this problem. See the Rule of Zero for more. Which path you chose is a decision you must make.

Problem 2

const Book& Library::borrow_book(const std::string& isbn){
    for(int i = 0; i < _maxNumberBook; i++){
        if(_book[i].query_isbn(isbn)){
            _numberOfBorrows[i] = _numberOfBorrows[i] + 1;
            return _book[i];
        }
    }
    std::cout << "ISBN with " << isbn << " does not exist" << std::endl;
}

Does not return a Book & in all cases. If the book is not found, callers to borrow_book don't get nothing back, they get something completely undefined. This error is also flagged by modern compilers.

There are a couple common solutions here. Returning std::optional or a std::pair<bool, const Book*> both provide something that can be tested for a valid result. Returning const Book *allows returning nullptr which again can be tested for a valid Book. The function could also return bool and update a parameter.

It could be my upbringing in C, but I think returning a raw, unowned pointer is most appropriate here, but this is opinion.

const Book * Library::borrow_book(const std::string& isbn){
    for(int i = 0; i < _maxNumberBook; i++){
        if(_book[i].query_isbn(isbn)){
            _numberOfBorrows[i]++;
            return &_book[i];
        }
    }
    std::cout << "ISBN with " << isbn << " does not exist" << std::endl;
    return nullptr;
}

Side note: Because _maxNumberBook is not tied in any way to the number of books actually stored in the library, for(int i = 0; i < _maxNumberBook; i++){ is high risk. _book[i] may not have been set to a defined value.

user4581301
  • 33,082
  • 7
  • 33
  • 54