10
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
3
  • 12
    \$\begingroup\$ The title misled me initially - I was expecting a library of console functions! \$\endgroup\$ Dec 13 '21 at 13:14
  • \$\begingroup\$ Very good form for a new developer. I agree with the majority of the advice below. One more minor thing. Surround all of your code in your headers with the #ifndef logic. If you do not need to pull in a class, then you do not need to pull in its dependancies. \$\endgroup\$
    – mreff555
    Dec 15 '21 at 0:36
  • \$\begingroup\$ @mreff555 thanks bro, appreciate it 😌 \$\endgroup\$
    – Pham Long
    Dec 15 '21 at 3:40
17
\$\begingroup\$

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.

\$\endgroup\$
8
  • \$\begingroup\$ @TobySpeight Thanks, I forgot about that. I changed the wording, I hope this is better. Now I wonder whether making a Book's properties immutable is the right thing to do, or if it's better to just have getters and no setters. \$\endgroup\$
    – G. Sliepen
    Dec 13 '21 at 14:23
  • \$\begingroup\$ The other factor that influences the choice is whether you want the objects to be assignable. const members inhibit the automatic generation of operator=() for the class. As nothing here uses it, that's fine; it does lead us to ask some design questions (such as whether we care about object identity or value, for instance). I'm a bit stretched today, or I might write my own answer looking at that. \$\endgroup\$ Dec 13 '21 at 15:56
  • 1
    \$\begingroup\$ I was surprised I had to scroll down this far for someone to finally point out that logic and I/O should be separated, thanks Sir! Beyond the obvious Single Responsibility Principle, I would also emphasize testability. The OP didn't show any test, so I guess they don't have any in the first place; however I've many time seen large "upper" layers too tied to I/O to be tested and everyone just shrugging it off as "it cannot be tested"... and that's a poor habit to get into. \$\endgroup\$ Dec 13 '21 at 16:30
  • \$\begingroup\$ Most of these are the things that I noticed but always forgot along the way. Thank you for the very detailed feedback ^_^ \$\endgroup\$
    – Pham Long
    Dec 13 '21 at 18:49
  • 1
    \$\begingroup\$ making members const will disable move semantics, I suggest omitting const \$\endgroup\$
    – A. Smoliak
    Dec 14 '21 at 11:15
6
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ Some of the IO-operations I have no clue how to check (like vector push_back function for example) but I'll note that. Can you explain why is the brace initialization is better? Thank you \$\endgroup\$
    – Pham Long
    Dec 13 '21 at 2:31
  • \$\begingroup\$ Sure, you can get a detailed view here \$\endgroup\$ Dec 13 '21 at 3:46
  • 1
    \$\begingroup\$ Vector push_back is not considered IO, it is rather in-memory handling of a container. For IO you will most likely use open, close, write, read or c++ streams to files/devices. But you can enable exceptions for streams. For a user executable it is better to convert it to a readable error expression for a library user it may be a custom one or the one thrown. If you need to convert it to anyway, you can just check the return codes. \$\endgroup\$ Dec 13 '21 at 9:55
  • \$\begingroup\$ @Ritualmaster I mean the IO operations should be based on the result of the action before it (in this case, pushing an object to the vector) but idk how to check that because push_back is a void function. About the IO streams, you're absolutely correct. I should have used them only for files input output, and should have had separate read and print functions for console outputting. Thank you for the wise words. \$\endgroup\$
    – Pham Long
    Dec 13 '21 at 18:34
  • 1
    \$\begingroup\$ @JDługosz Thank you. I read some opinions and got the idea of it. \$\endgroup\$
    – Pham Long
    Dec 15 '21 at 3:38
2
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
9
  • \$\begingroup\$ Hey, thanks for the commet. I can only use C++ 11 features for now and so far using a getchar is the only way I can think of to get rid of the new line character. Can you suppose a better solution for C++ 11 and previous? \$\endgroup\$
    – Pham Long
    Dec 14 '21 at 3:57
  • \$\begingroup\$ @phamlong if you're sure you're always getting the newline character, you can just use substr and take until the last character. \$\endgroup\$ Dec 14 '21 at 9:12
  • \$\begingroup\$ @PhamLong, I have updated my answer with an example showing how to use it. \$\endgroup\$
    – jdt
    Dec 14 '21 at 12:35
  • 1
    \$\begingroup\$ @TamoghnaChowdhury, after calling std::cin >> num there is still a newline in the console buffer - see here for an example: onlinegdb.com/XYQ7hDdhf. I'm not sure how substr will help? \$\endgroup\$
    – jdt
    Dec 14 '21 at 16:22
  • 1
    \$\begingroup\$ @TamoghnaChowdhury using substr on string_view is just fine, along with remove_prefix and remove_suffix. General substr on string might be unnecessary; see if you can construct a string_view instead; or to trim the string on the right you can change its length. \$\endgroup\$
    – JDługosz
    Dec 15 '21 at 15:28
2
\$\begingroup\$

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;

\$\endgroup\$
8
  • 3
    \$\begingroup\$ "<cringe> 😬 It is just plain silly to write:" Even if you feel strongly about this, maybe don't put it this harshly to a newbie. \$\endgroup\$ Dec 13 '21 at 16:06
  • \$\begingroup\$ I was taught that it's the way to have default parameters in the constructor. string_view is a new feature in C++ 17 I believe and we're using C++ 11 in my course. I'll note that for future usages. About the == operator, you're right. I could have written it cleaner. Thanks for the feedback 😄 \$\endgroup\$
    – Pham Long
    Dec 13 '21 at 18:30
  • \$\begingroup\$ @TamoghnaChowdhury It's OK he put an emoji there, I suppose it's nothing serious. That block of code looks a bit stupid when I look back at it tbh 🥲 \$\endgroup\$
    – Pham Long
    Dec 13 '21 at 18:31
  • 2
    \$\begingroup\$ Emoji: Just trying to communicate better with students who are less than half my age. And it does help convey the relative importance of the issues and helps you learn from it, at least in this case. In an in-person code review you'd get a pie in the face. \$\endgroup\$
    – JDługosz
    Dec 14 '21 at 15:29
  • 1
    \$\begingroup\$ I imagine you have a fridge full of pies in your office ;-) \$\endgroup\$
    – jdt
    Dec 14 '21 at 17:11
1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ I like the idea of using function pointers but I think that your implementation will make a good follow-up question. \$\endgroup\$
    – jdt
    Dec 15 '21 at 14:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.