Console Library System in C++

Question

I'm a beginner and came up with this simple project in C++. Any feedback would be very much appreciated.

GitHub Repo

Note: I'm using a file called "db.txt" as a database.

main.cpp

#include "Library.h"
#include <iostream>
#include <vector>
#include <algorithm>
#include <fstream>

int main() {
    Library::init(); //initiates program    
    Library::run(); //run program
    return 0;
}

Book.h

#pragma once
#include <string>
#ifndef BOOK_H
#define BOOK_H
class Book {
    std::string title;
    std::string author;
    std::string category;
    bool available;
public:
    Book(std::string title_ = "", std::string author_ = "", std::string category_ = "", int avai = 1);
    void changeAvailability();
    bool getAvailability();
    std::string getTitle();
    std::string getAuthor();
    std::string getCategory();
    friend std::ostream& operator << (std::ostream& os, const Book& book);
    friend std::istream& operator >> (std::istream& is, Book& book);
    friend bool operator == (const Book& b1, const Book& b2);
};
#endif

Book.cpp

#include "Book.h"
#include <iostream>
#include <string>


Book::Book(std::string title_, std::string author_, std::string category_, int avai) : title(title_), author(author_), category(category_), available(avai) {}


void Book::changeAvailability() {
    available = !available;
}


std::string Book::getTitle() {
    return title;
}

std::string Book::getAuthor() {
    return author;
}

std::string Book::getCategory() {
    return category;
}

bool Book::getAvailability() {
    return available;
}

std::ostream& operator << (std::ostream& os, const Book& book) {
    std::string avai = (book.available) ? "(Available)" : "(Unavailable)";
    os << book.title + " - " + book.author + " " + avai + "\nCategory: " + book.category << std::endl;
    return os;
}

std::istream& operator >> (std::istream& is, Book& book) {
    std::cout << "Name of the book: " << std::endl;
    std::getline(is, book.title);
    std::cout << "Name of the author: " << std::endl;
    std::getline(is, book.author);
    std::cout << "Book's category: " << std::endl;
    std::getline(is, book.category);
    return is;
}

bool operator == (const Book& b1, const Book& b2) {
    if (b1.title == b2.title and b1.author == b2.author and b1.category == b2.category) {
        return true;
    } 
    return false;
}

Library.h

#include <vector>
#include <string>
#include "Book.h"
#ifndef LIBRARY_H
#define LIBRARY_H

//namespace singleton design
namespace Library {
    namespace Container {
        static std::vector<Book> db;
    }
    void printAllBooks();

    void addBook();

    void deleteBook();

    void searchByCategory();

    void borrowBook();

    void returnBook();

    void writeToCustomFile();

    void init();

    void readFromFile(std::string filename = "db.txt");

    void writeToFile(std::string filename = "db.txt");

    int takeInput();

    void run();
}
#endif

Library.cpp

#include "Library.h"
#include "Book.h"
#include <iostream>
#include <vector>
#include <algorithm>
#include <fstream>
#include <string>

using namespace Library::Container;

//function print all books from library 
static void Library::printAllBooks() {
    if (db.size() == 0) {
        std::cout << "There are no existing book. Please start by adding new books!" << std::endl;
        return;
    }
    else {
        int order = 1;
        std::sort(db.begin(), db.end(),
            [](Book b1, Book b2) {
                return b1.getAvailability() > b2.getAvailability();
            }
        );
        for (auto& i : db) {
            std::cout << order++ << ". " << i << std::endl;
        }
    }

}


//function to add new book to library
static void Library::addBook() {
    Book book;
    std::cin >> book;
    db.push_back(book);
    std::cout << "Book added successfully!" << std::endl;
    writeToFile();
}


//function to delete book from library
static void Library::deleteBook() {
    Book book;
    std::cin >> book;
    auto it = std::find(db.begin(), db.end(), book);
    if (it == db.end()) {
        std::cout << "Cannot find the book! Please try again!" << std::endl;
        return;
    }
    else {
        std::cout << (*it).getTitle() + " by " + (*it).getAuthor() + "has been deleted!" << std::endl;
        db.erase(it);
        
    }
    writeToFile();
}


//function to search books by category
static void Library::searchByCategory() {
    int order = 1;
    std::sort(db.begin(), db.end(),
        [](Book b1, Book b2) {
            return b1.getCategory() < b2.getCategory();
        }
    );
    std::string query;
    std::cout << "Which book category are you looking for?" << std::endl;
    std::getline(std::cin, query);
    if (query == "") {
        printAllBooks();
    }
    std::string copy_query = query;
    copy_query[0] = std::toupper(query[0]);
    for (auto& i : db) {
        if (i.getCategory().find(query) != std::string::npos || i.getCategory() == copy_query) {
            std::cout << order++ << ". " << i << std::endl;
        }
    }
    if (order == 1) {
        std::cout << "There is no book with this category!" << std::endl;
    }
}


//function to borrow a book
static void Library::borrowBook() {
    Book book;
    std::cin >> book;
    std::vector<Book>::iterator it = std::find(db.begin(), db.end(), book);

    if (it == db.end()) {
        std::cout << "Cannot find book! Please try again!" << std::endl;
        return;
    }
    else if ((*it).getAvailability() == false) {
        std::cout << "Book is being borrowed! Sorry." << std::endl;
    }
    else {
        (*it).changeAvailability();
        std::cout << "You are now borrowing " + (*it).getTitle() + " by " + (*it).getAuthor() + ". Enjoy the book :)" << std::endl;
    }

    writeToFile();
}


//function to return book after borrowing
static void Library::returnBook() {
    Book book;
    std::cin >> book;
    std::vector<Book>::iterator it = std::find(db.begin(), db.end(), book);

    if (it == db.end()) {
        std::cout << "Cannot find book! Please try again!" << std::endl;
        return;
    }
    else if ((*it).getAvailability() == true) {
        std::cout << "Book is not being borrowed!" << std::endl;
    }
    else {
        (*it).changeAvailability();
        std::cout << (*it).getTitle() + " has been returned!" << std::endl;
    }

    writeToFile();
}


//write to custom file
void Library::writeToCustomFile()
{
    std::string filename;
    std::cout << "What file do you want to write to?" << std::endl;
    std::getline(std::cin, filename);
    std::ofstream ofile(filename);
    if (!ofile.is_open()) {
        std::cout << "An error occured while opening file for writing. Please try again!" << std::endl;
        return;
    }

    for (auto i : db) {
        ofile << i << std::endl;
    }
}


//init function
void Library::init() {
    db.clear();
    Library::readFromFile();
}



//function to read from a file. Can read from custom file (will be implemented later)
static void Library::readFromFile(std::string filename) {
    std::vector<std::string> class_mem;
    std::ifstream ifile(filename);
    if (!ifile.is_open()) {
        std::cout << "An error occured while opening file for reading. Please try again!" << std::endl;
        return;
    }
    std::string tmp;
    while (std::getline(ifile, tmp, ',')) {
        class_mem.push_back(tmp);
        if (class_mem.size() == 4) {
            Book book(class_mem[0], class_mem[1], class_mem[2], std::stoi(class_mem[3]));
            db.push_back(book);
            class_mem.clear();
        }
    }

}


//function to write to a file. Can write to custom file (will be implemented later)
static void Library::writeToFile(std::string filename) {
    std::ofstream ofile(filename);
    if (!ofile.is_open()) {
        std::cout << "An error occured while opening file for writing. Please try again!" << std::endl;
        return;
    }
    if (filename == "db.txt") {
        for (auto i : db) {
            ofile << i.getTitle() + "," + i.getAuthor() + "," + i.getCategory() + "," + std::to_string(i.getAvailability()) + ",";
        }
    }
    else {
        for (auto i : db) {
            ofile << i << std::endl;
        }
    }

}


//function to take user's input
int Library::takeInput() {
    int input(0);
    std::cout << "MENU:\n1: Print all books\n2: Add a new book to library\n";
    std::cout << "3: Delete a book\n4: Search for books of a specific category\n5: Borrow a book\n6: Return a book\n";
    std::cout << "7: Write to custom file\n8: Exit\n\nNOTE : ENTERING -1 WILL CAUSE THE PROGRAM TO STOP AUTOMATICALLY" << std::endl;
    std::cin >> input;
    if (!std::cin) {
        return -1;
    }
    int temp = std::getchar();
    return input;
}


//main program
void Library::run() {
    bool userExit = false;
    while (!userExit) {
        switch (Library::takeInput()) {
        case -1:
            std::cout << "Program exited due to invalid input." << std::endl;
            userExit = true;
            break;
        case 1:
            Library::printAllBooks();
            break;
        case 2:
            Library::addBook();
            break;
        case 3:
            Library::deleteBook();
            break;
        case 4:
            Library::searchByCategory();
            break;
        case 5:
            Library::borrowBook();
            break;
        case 6:
            Library::returnBook();
            break;
        case 7:
            Library::writeToCustomFile();
            break;
        case 8:
            //save changes to file
            Library::writeToFile();
            userExit = true;
            break;
        default:
            std::cout << "Invalid input. Please input a number shown on the menu" << std::endl;
            break;
        }
    }
}

Answer 1

Make variables and functions const where appropriate

You used const references in a few places, but you missed a lot of opportunities to make other things const. Consider a Book for example. Ideally, once constructed, the title, author and category should not change. So you could make this explicit and mark these member variables as const. This allows the compiler to better optimize code, and prevents accidental mistakes in your code. The drawback is that you can then no longer have a friend operator>>() to read in a Book from a std::istream, although you could still make a function that just takes a std::istream reference, and have that function construct and return a Book.

Apart from variables, you can also make functions const. This tells the compiler that this function will not change any of the member variables. You should do this for all getters like getAvailability(), getTitle() and so on, and in Library you would use this for things like printAllBooks(), writeToFile() and so on. You make a function const by adding const after the arguments but before the body, like so:

class Book {
    ...
    std::string getTitle() const;
    ...
};

std::string Book::getTitle() const {
    return title;
}

Consider returning const references from getters

While it is fine for getAvailability() to just return a bool, consider that returning a std::string by value in functions like getTitle() will cause the string to be copied. However, you can make it return a const reference to the member variable title instead:

const std::string &Book::getTitle() {
    return title;
}

The caller can still use getTitle() like before without any modifications, but now no copy has to be made by getTitle().

Reduce responsibilities of classes and functions

It is good practice to reduce the responsibilities each class and function has. This keeps the code simpler and more generic. Consider your class Book. Apart from storing its immutable properties that define a book, like title, author and category, it also keeps track of whether it is available, and it knows how to read and write to iostreams. However, the latter responsibilities introduce problems:

• The available flag is not something that should be in Book. It is not a property of a book whether it is "available" or not. Rather, it should be something the Libary keeps track of. Consider a program where you want to manage two libraries, each maybe having the same book in its catalog. Which library does the available flag refer to?

• The operator<<() formats the properties of a book in a human-readable way, and operator>>() seems to expect a human looking at the standard output. However, what if you later want to add functions to read and write Books in JSON format? What if you don't want to read from std::cin, why is operator>>() still printing to std::cout?

Technically, the friend functions operator<<() and operator>>() that work on Books are not part of class Book, they are just global functions, so you can argue that they are not part of Books responsibility. But still, I would consider reducing the code in Book.h to be just this:

struct Book {
    const std::string title;
    const std::string author;
    const std::string category;
};

Make Library a class

Your Library, while currently a namespace, already looks very much like a class: it has state (Container::db) and associated functions that act on that state. I would therefore make it a proper class:

class Library {
    std::vector<Book> db;
public:
    void addBook(const Book &book);
    void deleteBook(const Book &book);
    ...
};

void Library::addBook(const Book &book) {
    db.push_back(book);
}

void Library::deleteBook(const Book &book) {
    std::erase(db, book);
}

...

Again, consider what the basic properties of a library are: it has a catalog of books it has, you can add and remove books to the catalog, you can browse and search for books, and you can borrow and return books. Printing and file I/O is a responsibility that is better placed outside class Library, for the same reasons I mentioned above for class Book. Also, run() should not be a member function. You might keep this into a free-standing function, or perhaps create a class Librarian whose responsibility it is to run a Library.

So how to ensure this still is a singleton? Well, if you just want one Library, just create one instance of it. This doesn't prevent you from creating more instances though, but did it need to be a singleton to begin with?

Dealing with multiple copies of the same book

Most real-world libraries have multiple copies of a given book in their catalog, especially for popular books for which it is likely that more than one person wants to borrow it at the same time. One problem with your code is that two books with the same title, author and category are indistinguishable from each other. This becomes a problem for your borrowBook() and returnBook() functions. A solution is to make sure that books get some unique ID, like a serial number. However, this will make searching for a book in the catalog a little bit harder; you can't just use the first result returned by std::find(), you might have to look for other possible copies as well if the first result is already borrowed for example.

Related to this, searching for books in a std::vector is slow, as it has to check every element in sequence. If you ensure the elements of db are always sorted, you can do better, and use std::equal_range() to find books. There are also other containers you could consider using.

Use \n instead of std::endl

Prefer using \n instead of std::endl, the latter is equivalent to the former, but it also forces the output to be flushed, which is usually not necessary and might hurt performance.

Many C++ tutorials attempt to use std::endl everywhere, but it not only hurts the performance, but using \n is for many programmers easier to read, especially for non-C++ programmers. For example in Java or C, there is only the \n escape sequence.

Write it->foo instead of (*it).foo

It is more idiomatic to write it->getAvailability() than (*it).getAvailability().

Missing error checking in I/O functions

Errors can happen while reading from and writing to a file at any time, not just when opening the file. It is therefore not enough to just check that is_open() returns true. What you should do instead is check after doing all file I/O whether the stream object is in the expected state. When reading from a file, after reading the whole file ifile.eof() should be true. If it is not, you encountered an error before reading the end of the file. When writing to the file, after writing everything you wanted, call ofile.flush(), and then check if ofile.good() is still true, or do this in one go, like if (!ofile.flush()) { /* handle error */ }. Note that for std::ofstream (not std::ostream), you should also call close() and check its result.

Answer 2

The review by G. Sliepen has already covered most of what I would have said, so this is intended to be a complementary review addressing points not mentioned there.

Eliminate unused variables

The variable temp in your Library::takeInput() function is defined but never used. If you are just using the getchar() as a pause and don't care about the actual value, just write getchar();. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Fix conflicting declarations

In the Library.cpp file, most of the functions are declared static but they have already been declared within the Library.h file and are therefore implicitly extern. Those are conflicting declarations that your compiler should warn you about. This problem is best resolved by converting this namespace into a proper class and not trying to force it to be a singleton, as G. Sliepen has already noted.

Use RAII

Whenever you find yourself writing code that looks like this:

Library::init(); //initiates program    
Library::run(); //run program

it should trigger a warning in your brain. What happens if the user tries to invoke run() before init()? Better design is to use RAII and make it so that functions, libraries and classes are easy to use correctly. In this case, changing Library to a class, and moving the code in init into a constructor means that the Library will be ready to use as soon as it exists.

Eliminate duplicate text

The string "db.txt" exists in three different places in two different files. Better would be to have it in a single named variable (perhaps a static constexpr variable in the Library class) and then refer to it where needed.

Don't write trivial getters and setters

The Book object has a changeAvailability and getAvailability public functions. This means that any other code or object can alter this data item. If that's the intent, better would be to simply make availability a public data member. Don't write Java in C++. See C.131 for details.

Answer 3

One thing you can do to improve the code is to adopt brace initialization{} instead of direct initialization of variables, also All IO-operations should be checked, if they worked or failed. So, a surrounding if would be better.

Answer 4

Reading numbers

I was a bit confused with the int temp = std::getchar(); until I realized that you are trying to swallow the new line character. I think a better way to do it would be to read the whole line and then convert it to an integer:

std::string line;
if (!std::getline(std::cin, line))
    return -1;
int input;
auto ret = std::from_chars(line.data(), line.data() + line.size(), input);
if (ret.ec != std::errc() || ret.ptr != line.data() + line.size())
    return -1;
return input;

You can also use strol to convert the string if you can't use the C++17 from_chars:

std::string line;
if (!std::getline(std::cin, line))
    return -1;
char* end;
long input = strtol(line.data(), &end, 10);
if (end != line.data() + line.size())
    return -1;
return input;

Magic numbers

Using numbers like in the following may be okay for small projects like this, but makes it very hard to maintain as they grow in complexity:

switch (Library::takeInput()) {
case -1:
    std::cout << "Program exited due to invalid input." << std::endl;
    userExit = true;
    break;
case 1:
    Library::printAllBooks();
    break;
case 2:
    Library::addBook();
    break;

A better approach would be to create an enumeration like this:

enum class LibFunc
{
    InputError = -1,
    PrintAll = 1,
    AddBook = 2,
    DeleteBook = 3,
    SearchByCat = 4,
    BorrowBook = 5,
    ReturnBook = 6,
    WriteCustom = 7,
    Exit = 8
};

You can then modify Library::takeInput like so:

LibFunc Library::takeInput() {
    std::cout << "MENU:\n";
    std::cout << static_cast<int>(LibFunc::PrintAll)    << ": Print all books\n";
    std::cout << static_cast<int>(LibFunc::AddBook)     << ": Add a new book to library\n";
    std::cout << static_cast<int>(LibFunc::DeleteBook)  << ": Delete a book\n";
    std::cout << static_cast<int>(LibFunc::SearchByCat) << ": Search for books of a specific category\n";
    std::cout << static_cast<int>(LibFunc::BorrowBook)  << ": Borrow a book\n";
    std::cout << static_cast<int>(LibFunc::ReturnBook)  << ": Return a book\n";
    std::cout << static_cast<int>(LibFunc::WriteCustom) << ": Write to custom file\n";
    std::cout << static_cast<int>(LibFunc::Exit) << ": Exit\n";
    std::cout << "\nNOTE : ENTERING - 1 WILL CAUSE THE PROGRAM TO STOP AUTOMATICALLY\n";

    std::string line;
    if (!std::getline(std::cin, line))
        return LibFunc::InputError;

    int input;
    auto ret = std::from_chars(line.data(), line.data() + line.size(), input);
    if (ret.ec != std::errc() || ret.ptr != line.data() + line.size())
        return LibFunc::InputError;

    return static_cast<LibFunc>(input);
}

And Library::run to the following:

void Library::run() {
    bool userExit = false;
    while (!userExit) {
        switch (Library::takeInput()) {
        case LibFunc::InputError:
            std::cout << "Program exited due to invalid input." << std::endl;
            userExit = true;
            break;
        case LibFunc::PrintAll:
            Library::printAllBooks();
            break;
        case LibFunc::AddBook:
            Library::addBook();
            break;
        case LibFunc::DeleteBook:
            Library::deleteBook();
            break;
        case LibFunc::SearchByCat:
            Library::searchByCategory();
            break;
        case LibFunc::BorrowBook:
            Library::borrowBook();
            break;
        case LibFunc::ReturnBook:
            Library::returnBook();
            break;
        case LibFunc::WriteCustom:
            Library::writeToCustomFile();
            break;
        case LibFunc::Exit:
            //save changes to file
            Library::writeToFile();
            userExit = true;
            break;
        default:
            std::cout << "Invalid input. Please input a number shown on the menu" << std::endl;
            break;
        }
    }
}

Answer 5

Book(std::string title_ = "", std::string author_ = "", std::string category_ = "", int avai = 1);

Why are you passing strings by value ? If you meant to use the sink idiom for the constructor, you're not. The constructor is a special case for parameters that directly go into members; but otherwise, you traditionally pass strings by const reference but now should use string_view for passing strings into functions.

Don't use "" for an empty string. This causes it to run the constructor for const char* parameter, and it doesn't know ahead of time that it's actually the empty string! The efficient and idiomatic way to write it is simply {}.

Prior to C++17:

Book (const std::string& title_ = {}, const std::string& author_ = {}, const std::string& category_ = {}, bool avai = true);

Current best practice:

Book (std::string_view title_ = {}, std::string_view author_ = {}, std::string_view category_ = {}, bool avai = true);

"Sink" idiom (advanced, only for constructors):

Book (std::string title = {}, std::string author = {}, std::string category = {}, bool avai=true) 
: title{std::move(title)}, 
  author{std::move(author)},
  category{std::move(category)}, 
  available{avai}
 {}

(and, note that you don't have to give the arguments distinct different names from the members. The arguments are only referred to in the initialization list and it specifically supports having the parameter with the same name as the member. In the body (if you had anything there) you would refer to the members, which hide the original parameters.)

---

Same constructor:
why are you passing an int for the last parameter (number available?) but storing a bool in the class?

---

All the one-line or trivial member functions should be defined directly in the class, not in a separate CPP file. You are seriously hurting performance here, since the compiler won't inline the calls.

---

void readFromFile(std::string filename = "db.txt");
Again, don't pass the string by value. But in this case, use std::filesystem::path rather than string (or string_view).

---

bool operator == (const Book& b1, const Book& b2) {
    if (b1.title == b2.title and b1.author == b2.author and b1.category == b2.category) {
        return true;
    } 
    return false;
}

<cringe> 😬 It is just plain silly to write:

if (condition) return true;
else return false;

What you mean is just: return condition;

But in a new enough compiler, you don't have to write this at all.

bool operator == (const Book& b1, const Book& b2) = default;

Answer 6

jdt has already commented on your switch statement, but I find single-use enums like in his solution somewhat ugly.

Alternatively, one can use function pointers (-to-members) to write an explicit vtable, then iterate over it. Be warned: the result is extremely terse.

Here's a sketch assuming you've converted Library to a class as G.Sliepen suggested:

class Library
{
    /*other code*/
private:
    struct Action
    {
        void (Library::*action)();
        const char *const description;
    } constexpr menu_choices[] {
        { &PrintAll, "Print all books" },
        { &AddBook, "Add a new book to library" },
        /*etc.*/
        { &Exit,  "Exit" } //MUST BE LAST
    };
};

enum input_state
{
    success, exit, error
};

static input_state readInteger(int &retval)
{
    if (std::string line; std::getline(std::cin, line))
        if (auto const ret = std::from_chars(line.data(), line.data() + line.size(), retval);
            ret.ec != {} && ret.ptr - line.data() == size(line))
            return input_state::success;

    return input_state::error;
}

input_state Library::selectAndPerformAction()
{
    std::cout << "MENU:\n";
    std::ptrdiff_t id{0};
    for(auto const &choice : menu_choices)
        std::cout << id++ << ": " << choice.description << '\n';
    //id is now first invalid index
    int input;
    do
        if(input_state::error == readInteger(input))
            return input_state::error;
    while(input >= id && //Use short-circuiting to solve the "loop-and-a-half" problem
          (std::cout << "Invalid input!  Try again!\n", true));
    (this->*menu_choices[input].action)();
    //Exit is always the last option; see definition of menu_choices
    return input != id - 1 ? input_state::success : input_state::exit;
};

void Library::run()
{
    while(input_state::success == selectAndPerformAction())
        ; //Do nothing
}

Because it involved the fewest changes to your code structure, I've written this up awkwardly mixing two separate questions: "Did we encounter an input error?" and "Should we exit?" A little refactoring can separate these two ideas and eliminate the input_status datatype.

Since the main advantage of this format is its terseness, I've also omitted extra {} in favor of suggestive indentation and just generally gone for maximum cleverness. Your code style might be different.