3
\$\begingroup\$

I am doing a beginner c++ project, where you can add books, display books. I'm planning on adding other functionalities like searching the database and checking if a book is available for purchase. I was wondering if the code is actually following best practices.

#include <string>
#include <iostream>
#include <vector>

class book
{
    unsigned int id;
    std::string title;
    std::string author;
    std::string publisher;
    int quantity;
public:
    void add();
    void display();
};

// -----------------
// Global Variables
// -----------------

std::vector<book> books;



void book::add()
{
    std::cout << "Enter unique id for the book: ";
    std::cin >> id;
    std::cout << "Enter the title of the book: ";
    getline(std::cin, title);
    std::cout << "Enter the author of the book: ";
    getline(std::cin, author);
    std::cout << "Enter the publisher of the book: ";
    getline(std::cin, publisher);
    quantity = 1;
    std::cout << std::endl << std::endl << "Book Recorded Successfully" << std::endl << std::endl;
}

void book::display()
{
    for(auto b: books)
    {
        std::cout << "Name of book: " << b.title << std::endl;
        std::cout << "Name of author: " << b.author << std::endl;
        std::cout << "Quantity available: " << b.quantity << std::endl;
        std::cout << std::endl << std::endl << std::endl;
    }
}


// -------------------
// FUNCTIONS
// -------------------
void book_menu();

void main_menu()
{
    int c;
    std::cout << "********************************************************" << std::endl;
    std::cout << "              BOOKSHOP MANAGEMENT SYSTEM" << std::endl;
    std::cout << "********************************************************" << std::endl;
    std::cout << "      1.  BOOKS" << std::endl;
    std::cout << "      2.  EXIT" << std::endl << std::endl << std::endl;
    std::cout << "Enter your choice" << std::endl;
    std::cin >> c;
    switch (c)
    {
    case 1:
        book_menu();
        break;
    case 2:
        exit(1);
        break;
    default:
        std::cout << "Wrong Input" << std::endl << std::endl << "Invalid Input";
        break;
    }
    return;
}

void book_menu()
{
    int c;
    book b;
    std::cout << "***********************************************" << std::endl;
    std::cout << "              BOOK MENU" << std::endl;
    std::cout << "***********************************************" << std::endl;
    std::cout << "      1.  ADD" << std::endl;
    std::cout << "      2.  DISPLAY" << std::endl;
    std::cout << "      3.  BUY BOOKS" << std::endl << std::endl << std::endl;
    std::cin >> c;
    switch (c)
    {
    case 1:
        b.add();
        books.push_back(b);
        break;
    case 2:
        b.display();
        break;
    default:
        std::cout << "Wrong Input" << std::endl << std::endl << "Invalid Input";
        break;
    }
}

int main() 
{
    while(1)
    {
        main_menu();
    }
    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ You seem to be missing some #include lines necessary for the code to compile. I recommend that you add those in before you receive any answers (as you're not allowed to edit the code once that happens). \$\endgroup\$ Aug 30, 2022 at 7:25

2 Answers 2

2
\$\begingroup\$

One of the most important skills in C++ programming is being able to properly model your problem. In fact, that’s literally why Bjarne Stroustrup created C++; he was trying to model a distributed/client-server architecture, and found that C wasn’t good enough for proper modelling (and the languages that were good at modelling were slow).

So you are trying to model a book store, basically. Cool. The question is… how well have you modelled a book store?

Well, let’s see.

Suppose I want a copy of Frank Herbert’s “Dune”:

auto dune = book{};

… except… that's not actually a copy of “Dune”.

That’s just an empty, meaningless “book”. It has no title. No author. No nothing. It kinda exists, and doesn't exist, all at the same time.

In fact, there is no possible way for me to make a copy of “Dune”, because all the data members of book are private, and there are no constructors or setters.

We’re not off to a good start.

And now I do this:

dune.add();

Now… what does “Dune dot add” mean? It’s gibberish. Add what to “Dune”? Nothing, according to the function parameters. It’s not even something like dune.add(new_chapter), which would at least make some kind of sense.

And yet… what’s actually happened in dune.add() is that you created a copy of “Dune”—author, title, and all. But… and this is really crazy thing… you didn’t even add it to the inventory. book::add() doesn’t add anything.

This is already pretty terrible, but it’s about to get much worse.

Because, next, I do this:

dune.display();

Now, I would think that this would display “Dune” somehow. Maybe it will display the entire book’s contents, maybe just the title and author… no idea (which is why display() is not a great name).

But here’s the absolutely ridiculous thing. Calling dune.display() will actually display the entire contents of the book store inventory…

BUT IT WILL NOT DISPLAY “DUNE”!!!

Because dune.add() didn’t add “Dune” to the store, and now dune.display() won’t display “Dune”.

What a mess, eh?

The problem here is that you are not thinking about what you are coding. You are just blindly throwing stuff together, with no concept for how it all actually works. You’re not thinking about your problem, or the various pieces of it, or how they fit together.

Let’s take a look at the book class for example. A book HAS-A title. That makes sense. A book HAS-A author. And a book HAS-A publisher. That’s all good.

Now… does a book HAS-A ID? Does it? If I have a copy of “Dune”, it obviously has a title: “Dune”. It has an author: “Frank Herbert”. And it has a publisher. All these things are part of the book. You can’t have a book without all three of these things (in theory). A book without a title is not a complete book.

So the question is: can a book be complete without an ID? Is it ever possible for a book to have an author, a title, and a publisher… but not yet have an ID, because it’s not in your system yet?

The answer can be yes or no depending on your system, but either answer has consequences. You need to think about it.

But here’s a question that is less open to debate: Does a book HAS-A quantity?

Think about it. Is “quantity” a property… of a book? If someone sees you reading a book, they can ask you what its title, author, or publisher is… but if they ask “so, what’s that book’s quantity?”… isn’t the answer always going to be “one”? When is the quantity of a book not “one”?

You see? This is an example of the mess you end up with if you don’t really think about your objects or your models. “Quantity” is not a property of a book; “quantity” is a property of an inventory of books. Asking the book “Dune” what its quantity is makes no sense whatsoever… but asking a book store’s inventory what the quantity of “Dune” books is, that makes sense.

So let’s step back and think of what a book store actually is, so we can suss out what the models and their relationships should be.

A book store HAS-A inventory of books. So:

class store
{
    std::vector<book> inventory;
};

You want to be able to add new books to the store’s inventory:

class store
{
    std::vector<book> inventory;

public:
    auto add(book) -> void;
};

And you want to display the store’s inventory:

class store
{
    std::vector<book> inventory;

public:
    auto add(book) -> void;

    auto display_inventory() -> void;
};

And, maybe later, you want to be able to buy books by ID:

class store
{
    std::vector<book> inventory;

public:
    auto add(book) -> void;

    auto display_inventory() -> void;

    // Maybe later:
    //
    // You tell the store you want to buy this book ID, and it returns the
    // book you bought.
    auto buy(book_id) -> book;
    // Maybe you want to handle money, too, and return the book bought, and
    // the change:
    auto buy(book_id, cash) -> std::tuple<book, cash>;
    // Or maybe you want to be able to buy multiple books at once:
    auto buy(std::vector<book_id>, cash) -> std::tuple<std::vector<book>, cash>;
};

We’ll ignore buying for now, and focus on adding and displaying.

As you can see, both adding and displaying are things you do in the store… not in individual books.

So what does an individual book look like?

Well, this is a good start:

struct book
{
    std::string title;
    std::string author;
    std::string publisher;
};

What might you want to do with a book? Well, you might want to display its information. So:

struct book
{
    std::string title;
    std::string author;
    std::string publisher;

    auto display_information()
    {
        std::cout << "\""
            << title
            << "\", by "
            << author
            << " ("
            << publisher
            >> ")";
    }
};

Now with that, the store’s display function might look like:

auto store::display_inventory() -> void
{
    std::ranges::for_each(inventory, [](auto&& b)
    {
        b.display_information();
        std::cout << "\n";
    });
}

So far we haven’t considered IDs. So let’s rethink.

Instead of the inventory being just a list of all the books the store has, it could be a list of book IDs and quantities. This has two benefits. First, it makes it trivially easy to see how many copies of a book the store has, and second, it allows the store to keep track of books it does not have… that is, books with a zero quantity. (If you just use a vector of books, then when the store has zero copies of “Dune”, there will be no instances of “Dune” in the vector. So… how do you even know if you carry “Dune”?)

So if the inventory is just a list book IDs and quantities… like this:

std::map<book_id, int> inventory;

Then we also need a map of book IDs to the actual book information:

std::map<book_id, book> books;

(You could also simplify this by using a single map, like std::map<book_id, book_info> inventory;, where book_info is a struct holding a book and a quantity. But let’s put that aside for now.)

So now you’d display inventory like this:

auto store::display_inventory() -> void
{
    std::ranges::for_each(inventory, [](auto&& info)
    {
        auto&& [id, quantity] = info;

        books[id].display_information();

        std::cout << " (qty: " << quanitity << ")\n";
    });
}

This will print each book once, along with the quantity.

If you’re going to do things this way, you should probably have one function to add new books to the store—which will return the new book’s ID—and another function to add new copies of known books. So, like:

class store
{
    // ... [snip] ...

    auto create_new_book(book) -> book_id;

    auto add_new_copy(book_id) -> void;

    // and maybe, to add multiple copies at once:
    auto add_new_copies(book_id, int) -> void;

    // ... [snip] ...
};

As you can see, there is more than one way to do this. The point is that whatever model you use, it should make sense. If you do nonsense like “adding to books”, you will end up with nonsense. If you model your system sensibly—for example, “adding books to the store”—then things will work logically, and as you expand your system (adding buying, for example), it will expand in logical ways.

A good C++ coder spends 90% of their time thinking about the structure of their problem, the logical objects in it, and how they interact. A good C++ programmer doesn’t touch the keyboard or write a line of code until they’ve thoroughly mapped out the model of their system.

I would suggest starting from scratch, but this time, think about the elements of your problem. If you get the structure right, your code should read almost like plain English, like:

auto const id1 = store.find_by_title("Dune");
auto const id2 = store.find_by_title("Foundation");

// How many copies of each?
std::cout << "Number of copies of Dune: " << store.get_quantity_of(id1) << "\n";
std::cout << "Number of copies of Foundation: " << store.get_quantity_of(id2) << "\n";

// Buy a couple copies.
auto [books, change] = store.sell({id1, id2}, 100_dollars);

// How much change?
std::cout << "I have " << change << " in change.\n";

// What books did I buy?
for (auto&& book : books)
    book.display_information();

Code review

class book
{
    unsigned int id;
    std::string title;
    std::string author;
    std::string publisher;
    int quantity;
public:
    void add();
    void display();
};

This is a very badly designed class.

The way you use it is by declaring an “empty” book, and then later filling it with data by calling add(), like so:

void book_menu()
{
    int c;
    book b;
    // ... [snip] ...
    std::cin >> c;
    switch (c)
    {
    case 1:
        b.add();
        books.push_back(b);
        break;

Now, here’s the problem… what if you forget to do b.add() before books.push_back(b)? You will not only have added a meaningless “empty” book object to you the store, you will have added a book with a garbage ID, which could completely trash your store’s inventory in a number of ways.

A well-design class should never be in a broken state. It should be literally impossible to create a book object that isn’t legal. One way to do that is via constructors. For this class, you would need at least two: one to create wholly new books and get a new ID for them, and one to use the ID to get existing books. (A better idea would be to take the ID out of the book class, but that’s another issue.)

So, for example:

class book
{
    unsigned int id;
    std::string title;
    std::string author;
    std::string publisher;

public:
    explicit book(unsigned int i)
        : id{i}
    {
        // Look up ID in some map or something, and then load the title,
        // author, and publisher, or throw an exception if it’s an unknown
        // ID.
    }

    book(std::string t, std::string a, std::string p)
        : id{get_new_id()}
        , title{t}
        , author{a}
        , publisher{p}
    {
        // Make sure to put the book information in the ID map.
    }
};

With these constructors, it is now impossible to create an invalid, garbage book object.

// This will add Dune to the store’s book database (though the store should
// still have a quantity of zero until you *ACTUALLY* add it).
auto dune = book{"Dune", "Frank Herbert", "Publisher"};

// This will load the information for the book with ID 35, whatever that is:
auto some_book = book{35}

// There is literally no other way to make a book, and thus, no way to make a
// garbage book (unless you do so manually, which will be easy to spot).

But as I said, the ID really shouldn’t be part of the class. Nor should the quantity. Those are not properties of a book. If you take those out, you only need a single constructor, and you don’t need to tie the book class to the store’s book database (which is how spaghetti code begins).

void book::add()

This function should not be in book at all. It doesn’t even “add” anything.

void book::display()

It makes sense to have a function to display the information about a book. Unfortunately, that’s not what this function does.

If it did display information about a book, then it should be marked const, at least.

        std::cout << std::endl << std::endl << std::endl;

Never use std::endl. It’s always wrong.

What the line above does is:

  1. Write '\n' to std::cout.
  2. Flush std::cout (which is slow and expensive).
  3. Write '\n' to std::cout.
  4. Flush std::cout (which is slow and expensive).
  5. Write '\n' to std::cout.
  6. Flush std::cout (which is slow and expensive).

Except it’s even worse than that, because std::cout is often line-based anyway, which means it automatically flushes every time you write new line. So it may actually be:

  1. Write '\n' to std::cout.
  2. Flush std::cout (which is slow and expensive).
  3. Flush std::cout (which is slow and expensive).
  4. Write '\n' to std::cout.
  5. Flush std::cout (which is slow and expensive).
  6. Flush std::cout (which is slow and expensive).
  7. Write '\n' to std::cout.
  8. Flush std::cout (which is slow and expensive).
  9. Flush std::cout (which is slow and expensive).

That’s just absurd.

If you want to write a newline, just write a newline:

        std::cout << "\n" << "\n" << "\n";

Or just:

        std::cout << "\n\n\n";

A lot less typing. Thousands of times faster.

void main_menu()
{
    int c;
    // ... [snip] ...
    std::cin >> c;
    switch (c)
    {

    // ... [snip] ...

    default:
        std::cout << "Wrong Input" << std::endl << std::endl << "Invalid Input";
        break;
    }
    return;
}

You don’t actually do any real error handling here. To completely break your program, just try entering a letter instead of a number.

        exit(1);

Never, ever, ever use exit() (or std::exit()) in a C++ program.

(Also, returning 1 is not portable, but it generally means an error.)

I’ll get to what to do instead of exit() later.

    return;

There’s no need to manually return.

void book_menu()
{
    int c;
    book b;

This is bad practice, even in C these days. In C++ it’s especially bad, for even more reasons.

First, don’t list all a function’s variables at the top of a function. That hasn’t been necessary in C for at least 30 years, and it was never required in C++.

It also causes problems, because the longer a variable is in scope, the more likely it is to become corrupted, simply because the longer a variable hangs around, the more chances there are that someone could accidentally write to it when they didn’t mean to.

That’s especially bad when you’re using short names like c and b. In a long function, the probability approaches one that you will screw up and reuse a name, and change a variable’s value when you didn’t mean to, and end up with a nasty bug.

On top of that, it’s just silly. Oftentimes you don’t even need a variable. Like if you’re going to exit from the main menu, you never need a book object at all. So creating one is just wasteful.

The bottom line is this: Don’t create variables until you actually need them. Don’t let variables live any longer than is absolutely necessary. And don’t give variables short, meaningless names… especially if those variables are going to be hanging around a while.

Also, declaring variables this way is going out of style, because it is so dangerous. When you do int c;, c is uninitialized, and if you make the mistake of reading it, you will be triggering undefined behaviour (which could do anything, from crashing the problem, to appearing to work until a truly nasty bug surfaces later). You’re lucky that you don’t do that in your code… but you’re still playing with fire.

The modern way to do it in C++ is like this:

void book_menu()
{
    auto c = int{};     // this could also just be `auto c = 0;` if you prefer
    auto b = book{};

Except, of course, don’t declare the variables until you actually need them, so:

void book_menu()
{
    std::cout << "***********************************************\n";
    std::cout << "              BOOK MENU\n";
    std::cout << "***********************************************\n";
    std::cout << "      1.  ADD\n";
    std::cout << "      2.  DISPLAY\n";
    std::cout << "      3.  BUY BOOKS\n\n\n";

    auto c = int{};
    std::cin >> c;  // should check this for errors

    switch (c)
    {
    case 1: // should not use magic numbers, should use constants
        {
            auto b = book{};
            b.add();
            books.push_back(b);
        }
        break;
    case 2:
        {
            auto b = book{};
            b.display();
        }
        break;
    default:
        std::cout << "Wrong Input\n\nInvalid Input";
        break;
    }
}

You might also consider raw string constants, rather than a bunch of std::couts. Like this:

void book_menu()
{
    std::cout << R"(
***********************************************
              BOOK MENU
***********************************************
      1.  ADD
      2.  DISPLAY
      3.  BUY BOOKS


)";

Even better, you could put these strings in constants, like so:

using namespace std::string_view_literals;

constexpr auto book_menu_string = R"(
***********************************************
              BOOK MENU
***********************************************
      1.  ADD
      2.  DISPLAY
      3.  BUY BOOKS


)"sv;

void book_menu()
{
    std::cout << book_menu_string;

That allows you to separate the appearance of your menus from the actual functions.

int main() 
{
    while(1)
    {
        main_menu();
    }
    return 0;
}

The return 0; is not necessary.

You have created an infinite loop, with the idea that when your program wants to exit, it will call std::exit(). That is a terrible idea. std::exit() is a C library function, not a C++ function. It doesn’t work properly in C++. If you use it, you may end up with all kinds of craziness, all the way up to and including locking up your system completely.

(How could that happen? Easy. All you need is some resource that is supposed to be freed in some destructor, that if it is not freed, will remain locked up even after the program ends. Could be a lock file, for example. Then you call std::exit(), and, boom.)

All you need to do to fix all this is to simply return from main_menu(), saying whether to quit or continue. For example:

enum class main_menu_result
{
    quit,
    dont_quit
};

auto main_menu() -> main_menu_result
{
    std::cout << "********************************************************\n";
    std::cout << "              BOOKSHOP MANAGEMENT SYSTEM\n";
    std::cout << "********************************************************\n";
    std::cout << "      1.  BOOKS\n";
    std::cout << "      2.  EXIT\n\n\n";
    std::cout << "Enter your choice\n";

    auto c = int{};
    std::cin >> c;

    switch (c)
    {
    case 1:
        book_menu();
        break;
    case 2:
        return main_menu_result::quit;
    default:
        std::cout << "Wrong Input\n\nInvalid Input";
        break;
    }

    return main_menu_result::dont_quit;
}

auto main() -> int
{
    while (true)
    {
        if (main_menu() == main_menu_result::quit)
            return 0;
    }
}

That’s it. No more need for std::exit().

\$\endgroup\$
1
  • \$\begingroup\$ Please also note that std::exit does not destruct local variables with automatic storage duration, which can leak resources (file handles, file buffers, network connections, etc). \$\endgroup\$ Aug 30, 2022 at 22:14
0
\$\begingroup\$

One thing my teacher always required is to use const, for functions and class members as well. A book's title, author and publisher should all be const when they are created, good practice to keep it that way. Most of the things are mentioned by the answer above, I have a similar project that you could pay a visit and pick advices that work for you here

\$\endgroup\$
5
  • \$\begingroup\$ No, do not make class data members const. That is bad advice. Making data members const disables assignment operations, which makes the class hard to use in practice (often forcing you to use pointers just to move things around, which is inefficient). If you want an immutable object, make the members private, but not const, and add getters as necessary. (Marking member functions const where appropriate is fine, though.) \$\endgroup\$
    – indi
    Sep 4, 2022 at 22:07
  • \$\begingroup\$ I'd disagree since it makes sense for you to be not be using copy assignments. Making a class member private doesn't change anything but the fact that you can change the content through functions rather than doing it directly. Making class members const helps with code optimization. I agree on the fact that it disables some operator overloading though. \$\endgroup\$
    – Pham Long
    Sep 5, 2022 at 6:40
  • \$\begingroup\$ It makes sense to not be using copy assignment? 🤨 In what universe? I don’t think you understand how useless a class becomes with const data members. You might be able to put it in a vector, for example… but you can’t sort the vector, or perform any transformational algorithms on it (no remove(), reverse(), rotate()… even erase() might not work). There is a reason why the experts say not to make data members const. \$\endgroup\$
    – indi
    Sep 5, 2022 at 15:21
  • \$\begingroup\$ As for optimization, the claim that const helps with optimizing is mostly a myth. There are one or two minor benefits in specific places, but generally no, and in fact, const inhibits many of the most important optimizations. In general, if you use const excessively—like making data members const—your code will be slower, not faster. const is not an optimization tool, it is a tool to help the programmer not make silly mistakes. Making data members private does the same job, without the crippling usability and optimization costs. \$\endgroup\$
    – indi
    Sep 5, 2022 at 15:21
  • \$\begingroup\$ Here is proof that const data members make things worse. Note that not only is code much slower with const data members, it’s also more complex. \$\endgroup\$
    – indi
    Sep 5, 2022 at 15:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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