A library of books to demonstrate C++ to a C programmer 2

Question

I have previously asked A library of books to demonstrate C++ to a C programmer and I took some of the advise from user: JDlugosz and this is what I've managed to come up with so far:

My question(s) are relatively the same as in the original, however I would like to know if the changes that I've made based on some of the suggestions are appropriate, if I had missed anything, are there any mistakes, and is there any more room for improvement?

The only major internal changes are: instead of using multimap I decided to use unordered_multimap for better performance, and instead of auto generating a Book's ID number the user now has to type that in as well. I had to update a few of the functions in main.cpp in which I originally forgot to do. I originally posted this code with the ID change where the addBookInformation() function was updated, but forgot to update the unsigned to string for displayBook() and removeBook() functions... They are now updated as well to reflect the full change of using a string instead of an unsigned converted to a string. Now the ID is fully accepted by the user via console as a string. No need to convert to a string.

main.cpp

#include <string>
#include <iostream>
#include <iomanip>

#include "Library.h"

void displayMenu();
bool menuSelection( int option, Library* lib );
void addBookInformation( Library* lib );
void removeBook( Library* lib );
void displayBook( Library* lib );

int main() {
    Library lib;

    std::cout << "Welcome to the Library!\n";
    displayMenu();

    int option = 0;
    do {
        std::cout << "\nChoose an option from the menu ";
        std::cin >> option;
        if( std::cin.fail() ) {
            std::cout << "\nPlease enter an integer\n";
            std::cin.clear();
            std::cin.ignore( std::numeric_limits<std::streamsize>::max(), '\n' );
        }
        std::cout << '\n';
    } while( menuSelection( option, &lib ) );

    std::cout << "\nPress any key and enter to quit.\n";
    std::cin.get();
    return 0;
} 

void displayMenu() {
    std::cout << "================================================\n";
    std::cout << "1: Add a book to the library\n";
    std::cout << "2: Remove book from the library\n";
    std::cout << "3: Display the number of books in the library\n";
    std::cout << "4: Display a books information\n";
    std::cout << "5: Display the list of all books\n";
    std::cout << "6: Display menu option\n";
    std::cout << "7: Exit the library and quit\n";
    std::cout << "================================================\n";
}

bool menuSelection( int option, Library* lib ) {

    switch( static_cast<unsigned int>(option) ) {
        case 1: {
            addBookInformation( lib );

            std::cout.flush();
            system( "clear" );

            displayMenu();

            std::cout << "\nYou have entered a book into the libray.\n";
            break;
        }
        case 2: {
            if( lib->totalBooks() > 0 ) {    
                removeBook( lib );

                std::cout.flush();
                system( "clear" );

                displayMenu();  

                std::cout << "\nYou have removed a book from the library.\n";
            } else {
                std::cout << "\nThere are no books in library.\n";
            }
            break;
        }
        case 3: {
            unsigned int numBooks = lib->totalBooks();
            if( numBooks != 1 ) {
                std::cout << "\nThere are " << numBooks << " books in our library.\n";
            } else {
                std::cout << "\nThere is 1 book in our library.\n";
            }
            break;
        }
        case 4: {
            displayBook( lib );
            break;
        }
        case 5: {
            unsigned int numBooks = lib->totalBooks();
            if( numBooks > 0 ) {
                lib->displayLibrary();
            } else {
                std::cout << "\nThere are no books to display.\n";
            }
            break;
        }
        case 6: {
            std::cin.ignore();
            std::cout.flush();
            system( "clear" );
            displayMenu();
            break;
        }
        case 7: {
            return false;
        }
        default: {
            std::cout << "\nInvalid selection please try again.\n";
            break;
        }
    }
    return true;
}

void addBookInformation( Library* lib ) {
    unsigned int year = 0;    
    std::string title, author, bookID;

    std::cin.ignore();

    std::cout << "Please enter the books ID: ";
    std::getline( std::cin, bookID );

    std::cout << "Please enter the books title: ";
    std::getline( std::cin, title );

    std::cout << "Please enter the books author: ";
    std::getline( std::cin, author );

    std::cout << "Please enter the books year: ";
    std::cin >> year;   

    Book book( title, author, year );
    lib->addBook( bookID, book );
}

void removeBook( Library* lib ) {
    unsigned int numBooks = lib->totalBooks();
    if( numBooks == 0 ) {
        std::cout << "\nThere are 0 books in library; nothing to remove.\n";
        return;
    }

    std::string id;

    std::cin.ignore();

    std::cout << "\nRemove book by ID\n";
    std::cout << "Enter the books ID: "; 

    std::getline( std::cin, id );
    lib->removeBook( id );
}

void displayBook( Library* lib ) {
    unsigned int numBooks = lib->totalBooks();
    if( numBooks == 0 ) {
        std::cout << "\nThere are 0 books in the library; nothing to display.\n";
        return;
    }

    std::string id;

    std::cin.ignore();

    std::cout << "\nFind book by ID(I)\n";
    std::cout << "Enter the books ID: ";

    std::getline( std::cin, id );
    Book* book = lib->findBook( id );
    if( book ) {
        std::cout << *book;
    } else {
        std::cout << "\nBook was not found.\n";
    }   
}

Book.h

#ifndef BOOK_H
#define BOOK_H

#include <string>
#include <iostream>
#include <iomanip>

class Book {
private:
    std::string  title_;
    std::string  author_;
    unsigned int year_;

public:
    Book() = default; 
    Book( std::string title, std::string author, unsigned int year ) :
        title_{ std::move( title ) },
        author_{ std::move( author ) },
        year_{ year } 
    {}

    std::string titleIs() const { return title_; }
    std::string authorIs() const { return author_; }
    unsigned int yearPublished() const { return year_; }
    void updateTitle( const std::string& title ) { title_ = title; }
    void updateAuthor( const std::string& author ) { author_ = author; }
    void updateYear( unsigned int year ) { year_ = year; }
};

bool operator==( const Book& first, const Book& other );
bool operator!=( const Book& first, const Book& other );
std::ostream& operator<<( std::ostream& out, const Book& book );

#endif // BOOK_H

Book.cpp

#include "Book.h"

bool operator==( const Book& first, const Book& other ) {
    return ( first.titleIs()  == other.titleIs()  &&
             first.authorIs() == other.authorIs() &&
             first.yearPublished() == other.yearPublished() );
}

bool operator!=( const Book& first, const Book& other ) {
    return ( first.titleIs()  != other.titleIs()  ||
             first.authorIs() != other.authorIs() ||
             first.yearPublished() != other.yearPublished() );
}

std::ostream& operator<<( std::ostream& out, const Book& book ) {
    out << std::setw( 15 ) << "Title: " << book.titleIs() << '\n'
        << std::setw( 15 ) << "Author: " << book.authorIs() << '\n'
        << std::setw( 15 ) << "Year: " << book.yearPublished() << '\n';
    return out;
}

Library.h

#ifndef LIBRARY_H
#define LIBRARY_H

#include "Book.h"
#include <unordered_map>

class Library {
public:
    typedef std::unordered_multimap<std::string, Book> LibraryMap
    typedef const std::unordered_multimap<std::string, Book> LibraryMapConst;

private:
    LibraryMap books_;
public:
    Library() = default;

    void addBook( const std::string& id, Book& book );
    void removeBook( const std::string& id );

    Book* findBook( const std::string& id );

    std::size_t totalBooks() const;

    void displayLibrary();
};

#endif // LIBRARY_H

Library.cpp

#include "Library.h"

void Library::addBook( const std::string& id, Book& book ) {
    books_.insert( std::move( std::pair<std::string,Book>( id, book ) ) );
}

void Library::removeBook( const std::string& id ) { 
    books_.erase( id );
}

Book* Library::findBook( const std::string& id ) {
    auto it = books_.find( id );
    return &it->second;
}

std::size_t Library::totalBooks() const {
    return books_.size();
}    

void Library::displayLibrary() {
    LibraryMapConst::const_iterator it = books_.begin();
    for( ; it != books_.end(); ++it ) {
        std::cout << "ID " << it->first << '\n'
            << it->second << std::endl;
    }
}

Answer 1

Before I get into the meat of the code review, I need to start by saying the whole concept you're working with seems to be misguided. You keep saying you want to show C programmers what a modern C++ program looks like to "ease" them into C++. But then you deliberately choose not to use modern C++ techniques - and in fact use bad practices.

Your logic for doing so is that you don't want to throw an entire complex Greek essay at them because that would be too much for them to swallow all at once. But that makes no sense. When teaching someone Greek, you don't teach them with an essay in mangled Greek with bad grammar and English words and phrasing mixed in. You teach them with a good Greek essay. If the essay is too complicated for beginners, the solution is not to make it crappy Greek. The solution is to just use a simpler essay.

If you want to teach someone C++ - whether a C programmer or not - you teach them C++. You don't teach them "C with classes".

Your fixation on "familiarity" is completely misguided. Most modern C++ constructs are not only smarter and safer than anything in C, they are easier to reason about and understand. You're actually making C++ harder by regressing to C constructs rather than using modern C++. You don't need to mimic what C code looks like to make a C programmer understand C++, you just need to explain the C++ code... which you should be doing in any case.

So I am going to review the code not just as C++ code, but as C++ intended to be a teaching instrument for people familiar with C.

Let's start with the Book class.

Book

There is not really a good case for making the title, author, and year private members. Book has no real invariants that require protecting the data members. You can't "break" a book modifying the title without modifying the author or year at the same time.

A C programmer would probably write Book as (ignoring the different namespaces in C):

struct Book
{
    char* title;
    char* author;
    int year;
};

To really illustrate how easy it is to transition from C to C++, you shouldn't make the Book class much more complicated than that.

struct Book
{
    std::string title;
    std::string author;
    int year;
};

And you're done.

You don't even need the constructors. You do need the comparison and insertion operators, of course. I'll get to them in a moment.

You may have noticed I made year an int rather than an unsigned int. There is general consensus among C++ experts that unsigned types should be avoided except for bit-twiddling or cases where you specifically want modulo arithmetic. Even the use of unsigned types for container sizes is viewed as a mistake. Your intention in using a negative year is probably avoid negative year values... but to what end? "32000" is probably just a invalid a year for a book's publishing date as "-3", and if "32000" is legit then "-3" is just as legit because it would be "3 BCE". Using an unsigned type for the year may also create nasty surprises when someone tries to do something like find the amount of time between two books' publishing dates.

There is no need to include <iomanip> or <iostream> in the header. Firstly, you only use the manipulators in the cpp file, and secondly you only reference std::ostream (and in any case <iostream> is the wrong header - you'd need <ostream>).

So basically, the entirety of your book.hpp should be:

#ifndef BOOK_H
#define BOOK_H

#include <string>

struct Book {
    std::string  title;
    std::string  author;
    int year;
};

bool operator==( const Book& first, const Book& other );
bool operator!=( const Book& first, const Book& other );
std::ostream& operator<<( std::ostream& out, const Book& book );

#endif // BOOK_H

That is what modern C++ looks like. Simple, elegant, clear with a minimum of all the boilerplate crap you'd need to accomplish the same thing in C. There's no need to over-complicate things. Especially in a simple teaching example.

As an aside, you've rejected using a namespace for - again - incoherent reasons. If you're teaching C++, teach C++. "It's standard practice to put all your stuff in your own namespace to avoid name collisions" is hardly going to be too much for a C programmer to follow. It's not like they're unaware of what name collisions are, they have more built-in namespaces in C than in C++ (there is no tag namespace in C++), and wrapping stuff in braces isn't going to blow their minds. Just teach them the right way to do things.

In the cpp file, you should really write operator!= in terms of operator==. And if you really want to show something cool, you could use the std::tie trick to do the comparison much more succinctly:

return std::tie(first.title, first.author, first.year) ==
       std::tie(other.title, other.author, other.year);

Once again, "this isn't how they do it in C" isn't a good argument, because this isn't C. This is a very common pattern in C++, and it's trivial to understand even for people who don't know C. You don't need to explain how the trick works - at least not right away. You just need to demonstrate it.

Library

I don't see any reason why LibraryMap needs to be part of the public interface. In fact, if the rest of the code is written properly, there's no reason for it at all.

LibraryMapConst has even less reason to be. It's only used once, internally, and that usage is incorrect anyway.

If you're illustrating a simple library concept, unordered_multimap seems a bit overkill. Do you really want multiple books with the same ID? I think what you really want is unordered_map. One ID -> one book.

Defaulting the default constructor is superfluous.

addBook should take id and book by value. This is a pure sink function. You gain absolutely nothing from using references, and in fact lose efficiency. Assuming you want a single ID to point to a single book, you will also probably need some way of signalling failure in the case of duplicate ID. This would be a good time to illustrate exceptions.

removeBook might also need a way of signalling failure, but that's less important.

findBook should be const. And the return type should definitely not be a pointer. This is a design decision, but you should decide what you want to say to the student. Does searching give you a copy of the book, or does it give you the actual book stored in the library, which you can then change willy-nilly as you please. The latter seems a bit dangerous.

I'm going to assume you're returning a copy of the book, in which case the correct return should probably be std::optional<Book>. (If you're returning a reference, you'd probably want an optional reference wrapper.)

(Another issue with findBook is that if it finds an ID, it only returns a single book, rather than all of the books with that ID. But I think that's because you don't really intend to use a multimap.)

displayLibrary should probably take an output stream by reference for flexibility. It would allow printing the library in an error message (std::cerr) or to a log file. You're already illustrating the concept in the Book insert operator, so there's no sensible reason not to do this. displayLibrary should also be const.

All this doesn't simplify Library as much as it did Book, but helps a little:

#ifndef LIBRARY_H
#define LIBRARY_H

#include <string>
#include <unordered_map>

#include "Book.h"

class Library {
private:
    std::unordered_map<std::string, Book> books_;
public:
    void addBook( std::string id, Book book );
    void removeBook( const std::string& id );

    std::optional<Book> findBook( const std::string& id ) const;

    std::size_t totalBooks() const;

    void displayLibrary(std::ostream&) const;
};

#endif // LIBRARY_H

(I would also suggest that there's no reason to keep repeating "Book" in all the function names. What else would you be adding to/removing from a library of books? library.add(book); seems better than library.addBook(book);. Similarly, Library::displayLibrary seems repetitive - why not Library::display?)

addBook's implementation has an unnecessary move. It could just be:

void Library::addBook( std::string id, Book book ) {
    books_.insert( std::pair<std::string, Book>( id, book ) );
}

The template arguments are unnecessary as of C++17:

void Library::addBook( std::string id, Book book ) {
    books_.insert( std::pair( id, book ) );
}

And of course, what you really want here is emplace, not insert:

void Library::addBook( std::string id, Book book ) {
    books_.emplace( id, book );
}

And with moves:

void Library::addBook( std::string id, Book book ) {
    books_.emplace( std::move(id), std::move(book) );
}

But of course, you don't handle the case where there's already a book with that ID.

void Library::addBook( std::string id, Book book ) {
    auto [ position, success ] = books_.emplace( std::move(id), std::move(book) );
    if (!success)
        // do something here
}

You could throw an exception (best option), or return false (less ideal), or something else.

findBook is the most problematic function in this class, as it not only has a terrible interface, it causes undefined behaviour. What you really want is something like:

std::optional<Book> Library::findBook( const std::string& id ) {
    auto it = books_.find( id );
    if (it == books_.end())
        return std::nullopt;
    return it->second;
}

displayLibrary is also pretty problematic, for a couple of reasons.

First, the use of LibraryMapConst::const_iterator is misguided. If you want a const_iterator, just use auto it = books_.cbegin();. Or even better, auto it = books_.begin();, because displayLibrary should be const anyway.

Second, initializing it outside of the loop is bad practice even in modern C. You don't use it outside of the loop, so it shouldn't be visible outside of the loop.

Third, you're using the old form for loop rather than range for. I know this is one of those things you think "helps" C programmers transition to C, but that's nonsense. C programmers do not write for loops for iterators from begin() to end() because C programmers do not have iterators, begin(), end(), or container classes. You are not helping C programmers by providing them a bad example in a misguided attempt to create some kind of chimera transitional language between C and C++.

This is what C++ looks like:

void Library::displayLibrary(std::ostream& out) const {
    for(auto&& [id, book] : books_)
        out << "ID " << id << '\n' << book << '\n';
}

"That's not what it looks like in C" is a non sequitur. You're teaching C++? Then teach it. "When you loop over the elements in a container you do for (auto&& element : container). Period. If the elements are compound types and you'd like to break them apart, you do for (auto&& [ subelement1, subelement2, ... ] : container). That's it. That's all you need to know. Go forth and code. Class dismissed."

Student: "But, but, teacher! That doesn't look like C!"

Teacher: "That's because it's not."

The main program

This is where most of the problems with this code lie. Let's start from the main() entry point and work our way down.

Right at the top of main you initialize the library. That's not a problem, but it illustrates bad practice in general. You should not initialize things before you need them, and you don't need the library until at least after the greeting.

Next up is what I think is one of the biggest design mistakes in this program: using a naked int and magic numbers for program options. The greatest advantage of C++ over C is its strong type system. At a bare minimum, you should be defining constants for the menu options. But what you really need here is an enum. Something like:

enum class menu_option {
    add_book = 1,
    remove_book,
    book_count,
    display_book,
    book_list,
    display_menu,
    quit
};

Then you'd make a helper function to get an option:

auto get_menu_option()
{
    while (true)
    {
        std::cout << "Choose an option from the menu ";

        auto option = 0;
        if (std::cin >> option)
        {
            switch (option)
            {
            case 1: return menu_option::add_book;
            case 2: return menu_option::remove_book;
            // ...
            default:
                std::cout << "Invalid selection please try again.\n";
            }
        }
        else
        {
            std::cout << Please enter an integer\n";
            std::cin.clear();
            std::cin.ignore( std::numeric_limits<std::streamsize>::max(), '\n' );
        }

        // I think it would be wise and helpful to add a reminder of
        // how to display the menu here, such as:
        //std::cout << "To display the menu options again, enter" <<
        //    menu_option::menu << '\n';
    }
}

And the main function becomes:

int main() {
    std::cout << "Welcome to the Library!\n";

    Library lib;
    displayMenu();
    while ((auto option = get_menu_option()) != menu_option::quit)
    {
        // ...
    }
}

The body of the while loop will simply be:

switch (option)
{
case menu_option::add_book:
    do_add_book(lib);
    break;
case menu_option::remove_book:
    do_remove_book(lib);
    break;
// ...
case menu_option::quit:
    [[fallthrough]];
default:
    std::cerr << "(should never get here!)\n";
}

And then you'd just need to implement the actual function carried out by each menu option (aside from "quit").

(Or, perhaps better, the body of the main loop should just be a single function call to something like do_option(option, lib) that implements the switch.)

displayMenu()

This function is mostly fine as is, but if you implement the menu option enum, you could change it to:

void displayMenu() {
    std::cout << "================================================\n";
    std::cout << menu_option::add_book    << ": Add a book to the library\n";
    std::cout << menu_option::remove_book << ": Remove book from the library\n";
    // ...

menuSelection()

This is the first of the functions that takes the library as a pointer rather than a reference. I know you've argued that this is somehow "better" for C programmers, but that argument just doesn't hold water. You're already using references all over your code (such as in the comparison ops for Book). Using pointers for something like this doesn't help C programmers. It just illustrates bad practice.

Moving on.

I'm not sure why the option is static_cast to an unsigned int. What is the purpose of that?

Within menuSelection(), you not only determine what is selected, you carry out basically every operation that your program does. That is as much bad design in C++ as it is in C. One function: one job. Every one of the operations here should be broken out into their own function. You do that partially for some cases. In actuality, the logic of each operation is all over the place - some of it is in menuSelection(), some of it is in other functions like addBookInformation().

case 1: {
    addBookInformation( lib );

    std::cout.flush();
    system( "clear" );

    displayMenu();

    std::cout << "\nYou have entered a book into the libray.\n";
    break;
}

All of the above shouldn't be handled in menuSelection() (it should be handled in addBookInformation()), because you're not taking into account that adding the book might have failed (due to a duplicate ID, for example). I'm also not sure of the point of doing displayMenu() here.

But the biggest problem is these two lines:

    std::cout.flush();
    system( "clear" );

First, it should be std::system.

Second, it shouldn't be in the program at all. This is not portable. It is bad practice to teach new C++ programmers something unportable. Even worse, this is dangerous, because some unsuspecting newbie could compile and run your code on a machine where clear is the command to wipe their whole hard drive.

If you really want to "clear the screen", write a function that just does a bunch of newlines. That's as close as you can get to a portable clear screen function. But I'd suggest just living with the fact that you can't clear the screen portably, and don't worry about it.

case 2:
    // ...

Once again, all of this should be in the removeBook() function.

case 3: {
    unsigned int numBooks = lib->totalBooks();
    if( numBooks != 1 ) {
        std::cout << "\nThere are " << numBooks << " books in our library.\n";
    } else {
        std::cout << "\nThere is 1 book in our library.\n";
    }
    break;
}

This actually serves as a good example of why auto rocks. totalBooks() returns a std::size_t, not unsigned int. On some platforms std::size_t is 64 bits, while unsigned int is 32. I believe Windows is one of those platforms. Not using auto here has introduced an integer truncation bug.

Also, this is a good use case for the more modern form of if:

case 3: {
    if( auto numBooks = lib.totalBooks(); numBooks != 1 ) {
        std::cout << "\nThere are " << numBooks << " books in our library.\n";
    } else {
        std::cout << "\nThere is 1 book in our library.\n";
    }
    break;
}

Case 4 is cool, case 5 should be a function. Then there's case 6:

case 6: {
    std::cin.ignore();
    std::cout.flush();
    system( "clear" );
    displayMenu();
    break;
}

What is the point of all this? I get that you're doing the flush and system("clear") to clear the screen, by why the ignore() call?

Really, all this needs to be is a call to displayMenu(). Just forget about clearing the screen because it's just a cosmetic effect that can't be done portably, and focus on teaching what can.

addBookInformation()

Once again, should use a reference, not a pointer.

Declaring all the variables at the top like this is bad practice even in modern C. Declare them right where they're needed. This is not only cleaner, it is safer, and more efficient.

Here is another call to cin.ignore() that doesn't seem to serve any purpose. Why do you assume there is a character to ignore?

There is no error checking done anywhere, particularly when reading the year.

At the end of the function where you finally construct the book, this would be a good place for move()s.

There is no error checking to see if someone tried to add a duplicate ID.

removeBook()

Once again, pointers.

At the top of the function you have mismatched types again. You should use auto here, or even better, forgo the variable completely:

if (lib.totalBooks() == 0)

Again, variables declared too early, and cin.ignore() for no apparent reason.

displayBook()

Same as before: pointers, mismatched types, unnecessary variables, variables declared too early, cin.ignore().

The meat of the function is at the end:

Book* book = lib->findBook( id );
if( book ) {
    std::cout << *book;
} else {
    std::cout << "\nBook was not found.\n";
}

I recommended changing findBook() to return a std::optional<Book>. If you used auto... as you should in modern C++... none of this code would need to be changed (except that lib-> should be lib., because you should be using references).

With changes, it should be:

if( auto book = lib.findBook( id ); book ) {
    std::cout << *book;
} else {
    std::cout << "\nBook was not found.\n";
}

Summary

• Don't use unnecessary stuff. Don't explicitly define default constructors or write accessors just because. Show the minimal code that does the job.
• You probably want a map, not a multimap, in your library.
• Watch out for error cases (duplicate IDs when adding, ID not found either when searching or removing, etc.)
• Use C++ constructs. Your arguments for teaching C programmers crappy C++ just don't hold water. You're not helping them. If some C++ construct is too difficult for C programmers to understand, the correct thing to do is use a simpler example that does not require that construct... not to teach them the wrong way to do things.
• Use the type system. Don't use raw ints or magic numbers when there is a semantic meaning, and especially when they are peppered across several functions (as your menu options are).
• One function, one job.
• Don't use unportable stuff in teaching code. It's dangerous in any case, and especially dangerous due to the fact you're dealing with newbies that may not recognize the gun you just gave them to shoot their foot with.
• Use auto. It is simpler, and it prevents bugs.

---

About maps vs multimaps

For a demo program, I wouldn't even get into the complexity of multiple books with the same ID. But, since you asked....

The way I see it, you have 3 design options. I'll go from the simplest to the most complex.

Probably the easiest thing you could do is have your library defined as:

unordered_map<string, tuple<Book, int>>

where the int stores the count.

That would probably be ideal for a book seller database, because as far as they're concerned, their entire inventory of a given title is identical, and they're only interested in how many they have in stock.

But a library might want to keep track of particular books, rather than just titles. For example, it may have multiple copies of a given title, but some are hardcover, some are large print, and so on. It might also want to keep track of the condition, to know if the only copies that have are in poor condition, or to detect when someone is returning a book after damaging it.

In that case, the library might want to extend the Book class:

struct Book
{
    enum class type
    {
        // different types of books (hardcover, large print, etc.)
    };

    enum class condition
    {
        // different conditions (like new, water damage, etc.)
    };

    string author;
    string title;
    int year;
    type type = type::trade_paperback;
    condition = condition::good;
    // and so on...
};

And in that case, you'd use an unordered_multimap as your container.

The third option is to separate the metadata information about a book (title, author, etc.) from the information about a specific instance of that book. In the design above, the author, title, and year are repeated for each instance of a book, even when they are the same for multiple copies. The stuff that varies from copy to copy are things like the condition.

So what you'd have in this case would be something more like:

struct BookMetadata
{
    string author;
    string title;
    int year;
};

struct Book
{
    // perhaps a pointer or reference to the book's metadata

    type type = type::trade_paperback;
    condition = condition::good;
    // and so on...
};

with the library as:

unordered_map<BookID, tuple<BookMetadata, vector<Book>>>

So a book ID maps to one instance of the book's metadata, and multiple copies.

This would probably be closest to how the library would be stored in a relational database.

The menu option enum

I left a lot of code out - especially for the things that I was just suggesting, that you could take or leave. If you're going to use an enum for the menu options, you'll need a few more things.

To support streaming the menu options, you'd either need to cast them to std::underlying_type_t<menu_option> or - better - write an inserter. The inserter is basically one line:

template <typename CharT, typename Traits>
auto operator<<(std::basic_ostream<CharT, Traits>& o, menu_option v) ->
    std::basic_ostream<CharT, Traits>&
{
    return o << static_cast<std::underlying_type_t<menu_option>>(v);
}

While you're at it, you should probably write the extractor, too. It's not much more complicated, and it's a good illustration:

template <typename CharT, typename Traits>
auto operator>>(std::basic_istream<CharT, Traits>& i, menu_option& v) ->
    std::basic_istream<CharT, Traits>&
{
    using underlying_type = std::underlying_type_t<menu_option>;

    auto tmp = underlying_type{};
    if (i >> tmp)
    {
        switch (tmp)
        {
        case static_cast<underlying_type>(menu_option::add_book):
            v = menu_option::add_book;
            break;
        // ...
        default:
            i.setstate(std::ios_base::failbit);
        }
    }

    return i;
}

With that, the get_menu_option() function simplifies to:

auto get_menu_option()
{
    while (true)
    {
        std::cout << "Choose an option from the menu ";

        auto option = menu_option::quit;
        if (std::cin >> option)
            return option;

        std::cout << "Invalid selection please try again.\n";
        std::cin.clear();
        std::cin.ignore( std::numeric_limits<std::streamsize>::max(), '\n' );
    }
}

cin and error handling

So you're peppering cin.ignore() everywhere because some of your input operations are line-based and others are value-based, and the cin.ignore() deals with value-based input that leaves a trailing newline.

The first problem with this tactic is that it doesn't actually work. It probably appeared to work because you didn't try anything underhanded with the input. For example, every time you entered a year or a menu option number, you just entered the number, then pressed enter. In that situation, cin will eat the digits then stop, so ignore() will eat the newline. No problem. But... try entering the year as "2018<space>" (or any character, like "2018!"). What will happen there is that cin will eat the digits, then stop, leaving the space and the newline in the buffer. ignore() then eats the space. But the newline is left in the buffer.

The second problem is that it... doesn't actually work. So long as you run the program interactively, you probably won't notice, because each time the ignore() is done, the input buffer either has a trailing newline in it (ignoring the issue above) or it's empty. If it's empty, no harm no foul. But if you try to pipe input to the program - very handy for testing! - you may notice odd behaviour: the first character of certain input strings is missing. That's because in that situation, the buffer is not empty, and not just holding a stray newline - there's actually data there.

If you're going to do line-based input (which is a good idea generally, for programs meant to be interactive), you should stick to it, rather than jumping back and forth between value- and line-based input. That means, when you read an int, you also clear out the rest of the line in one shot:

void addBookInformation( Library& lib ) {
    std::cout << "Please enter the books ID: ";
    auto bookID = std::string{};
    std::getline( std::cin, bookID );

    // ...

    std::cout << "Please enter the books year: ";
    auto year = 0;
    std::cin >> year;
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    // OR!:
    //auto year_str = std::string{};
    //std::getline(std::cin, year_str);
    //auto year = std::stoi(year_str);

    // ...
}

This may be controversial, but I generally advise beginners not to do any formatted input with cin. Formatted output with cout is fine, because the only thing that can fail in formatted output is that the output stream goes bad. But formatted input is fraught with problems.

You can actually see it in the code you have: basically every time you do a formatted input, you need to do error handling with cin.clear() and cin.ignore(). And the times that you don't do that, such as in addBookInformation(), you should be. Failing to do so might leave cin in a fail state, which could wreak havoc with the rest of the program.

And that's just the tip of the iceberg. There are a number of reasons why formatted input from cin is a pain.

• You don't know if cin is in text mode or binary mode, and you can't portably change it. (There are non-portable tricks to change cin to binary mode.)
• You don't know what the locale of cin is. You may think reading "123" into an int will give you the value 123, but someone might have installed an exotic locale that's in base 24, so it reads in as 627. Even without assuming absurdities, you can still have headaches. For example, you may intend to read in "10,22" as a 2D point... but some English locales ignore the comma in numbers (so you'll get the single number 1022), while other locales treat the comma as a decimal separator (so you get the single number 10.22).
• You don't know what state the flags of cin are in. If someone set cin to hex mode and forgot to unset it, you may end up with a confused user entering menu option "10", only to be told that "16 isn't a valid menu choice".

And of course, the hassle of putting cin back into a sane state after badly formatted input.

None of these problems are insurmountable, but you'll probably just end up using or reimplementing Boost's I/O state saver library.

What I recommend to beginners is to forget trying to do formatted input from cin. Instead do all formatted input like this:

auto str = std::string{};
getline(std::cin, str);

auto iss = std::istringstream{str};
// Set everything you need here - flags, locale, etc.
// For example:
iss.imbue(std::locale::classic());

auto res = T{}; // where T is the type you're inputting
if (iss >> res)
{
    // Here you can check to see if there is anything else in the input
    // buffer. For example, if someone entered:
    //     "123 foo<enter>"
    // you might want to say this is bad input for an int.

    return res;
}

// there was an error - signal it somehow

Naturally you'd wrap all that up in a function. I generally suggest a template function, like:

template <typename T>
auto input()
{
    // ...
}

So all your formatted inputs become:

auto year = input<int>();

That might be too much for a simple example program. But on the other hand, the input<T>() functions are very generally useful.

At the very least, if you're doing line-based input, you need to do it consistently. You have two options. You can do this:

auto read_int()
{
    auto s = std::string{};
    getline(std::cin, s);

    return std::stoi(s);
}

and use that everywhere you read ints.

Or you can make an ignore_rest_of_line() function that clears the fail flag and ignores until '\n' (or verifies there's nothing but whitespace until the end of the line), and use that after every formatted input (preferably after checking for bad input). I know that sucks, but the IOstreams library is objectively horrible and long overdue for replacement, and that's just something we have to deal with.

Answer 2

Top Level

You are not using a namespace.
Put your code in our own namespace. You could try Cugler as the top level namespace. This will ensure your code is in a unique namespace.

Your use of pointers is worrying.

bool menuSelection( int option, Library* lib );
void addBookInformation( Library* lib );
void removeBook( Library* lib );

Normally in C++ we don't use pointers at all (this is one of the major changes from C). When we want to pass (without copying) we normally pass by reference this indicates that you are asking the function to use an existing object that it does not own (and therefore does not need to manage).

So the above functions should be declared as:

bool menuSelection( int option, Library& lib );
void addBookInformation( Library& lib );
void removeBook( Library& lib );

From the comments:

For example it is still valid code to pass a pointer to a function. This is one thing that C and C++ still has in common, however the preferred way of passing as you said would not be by pointer but by reference. I'm not disagreeing on this fact, but one has to consider the fact of those who are coming from C. You need a piece of transition code..

Though I agree that pointers are valid (which is shown by the code compiling). I completely disagree with the sentiment. The problem here is that in C (and C++ to an extent) is that pointers have no ownership semantics. The "Most" important point in C++ is the introduction of ownership semantics into the language as a concept. The owner of an object is the person responsible for deleting it. In C there is no concept of ownership and as a result has a legacy of being very buggy when it comes to resource management. In C++ we have tightened up resource management, we use references to show no ownership transfer and we use smart pointers to show the transfer of ownership (there are a few more helper class but you find those as you expand your C++ knowledge). The problem with pointers is that there is no explicit ownership semantics involved. You can make assumptions that passing pointers passes ownership and thus you should deleted them (but in this case that would fail). Pointers should be managed by a class (There ownership is defined by the class) and you can then pass objects of that class around without worrying about resource management as the class will do that for you. Within a resource management class there is no need to pass the pointer around.

Summary: I completely disagree with the implied use of pointers made in this comments. Pointers should (with rare exception) my maintained by a class object and never passed around a program.

The second major thing I notice is that your code is not const correct. We pay a lot more attention to const correctness in C++.

void displayBook( Library* lib );
                  //     ^  here the libray could potentially be
                  //        modified. But the I would not expect
                  //        a `display()` function to modify the
                  //        object being displayed.

I would have declared it like this:

void displayBook( Library const& lib );
                       // ^^^^^^  notice the const reference when
                       //         the function should not modify the
                       //         object

Your main is relatively short.
But I would have made it even shorter. Display a title then call a function that does all the work (then no return). The reason for calling a function is that makes the code more self documenting. The name of the function describes what you are going to do:

Like this:

int main() {
    std::cout << "Welcome to the Library!\n";

    try {
        Library lib;
        runTheLibrary(lib)
    }
    // Catch to force a stack unwind on exception.
    // But re-throw to force OS to notice the error.
    catch(std::exception const& e) {
        std::cout << "Exception: " << e.what() << "\n";
        throw;
    }
    catch(...) {
        std::cout << "Exception: Unknown\n";
        throw;
    }
    // No need for a return
    // Main is special. If you don't return it will add return 0
    // By not returning anything it indicates that main can only
    // return 0. If I see a main with a return I start looking for
    // other returns in main to see what the error conditions are.
    // By not returning at the end you indicate there are no other
    // returns in main and thus you don't need to look for them.
}

There are a couple of functions that I would have expected to be members of a class.

• runTheLibrary() or the code you put in main:
  I would have expected this to be a member of Library.
• menuSelection() I would have expected this to be a member of Library.
• addBookInformation() I would have expected this to be a combination of Library and Book.
• removeBook() I would have expected this to be a member of Library.

In the comments:

I see a majority of people from C++ state no; show the C people full C++. This would be like handing someone who never spoke Greek in their life and giving them an Essay in Greek and handing them a Harvard version of Greek Grammar and telling them to figure it out. I'm trying to do a more subtle approach.

I disagree with you and agree with the majority of C++ people teaching C++. Unfortunately the commenter (and probably the OP) think that C and C++ are very similar languages. Unfortunately this is not the case. They had similar roots and a similar syntax and you can incorrectly write C++ that looks like C. But to do so does no service to the person learning. We have a lot of examples of C programers writing C with a few classes thrown in, it so common there is even a term for this type of code C with Classes. Unfortunately no C++ programmer would consider this style of code good C++ and it inherently has the worst of both languages.

In summary learning C before learning C++ probably causes more problems because the common idioms of C are nothing like the common idioms of C++ and thus cause a very bad style of coding. You need to throw out your C way of thinking and learn to thing in the new C++ way to get the real power of the language.

Code Review

In this function:

bool menuSelection( int option, Library* lib )

There is lots of code in each case statement. This makes the function a bit long. Each case statement should simply call a function and break. By splitting this code out into its own function you are writing self documenting code (where each function name explains what it is doing).

In this function:

void addBookInformation( Library* lib ) {

You expect the function to know how a book is built. This tightly couples this function to the Book class. With such a tight coupling I would make the part of getting the information a part (or friend) of the class, thus explicitly documenting the tight coupling.

But I would also define it in such a way that we use standard C++ operators to read a book.

void addBookInformation( Library& lib ) {

    Book  aBook;
    std::cin >> aBook;   // Read a book using `>>`
                         // The input operator is tightly coupled to
                         // Book so it should know what to read.

    // Now lets add the book information to the Library.
    lib.addBook(std::move(aBook));
}

Don't declare variables at the top of a function.

    unsigned int year = 0;    
    std::string title, author, bookID;

You should get in the habit of declaring variables just before they are used for the first time. Also one variable per line please.

This is a good basic start for guards.

#ifndef BOOK_H
#define BOOK_H

But you may want to add your namespace here to make sure it is unique.

#ifndef CUGLER_BOOK_H
#define CUGLER_BOOK_H

Not sure I like having a default constructor.

    Book() = default; 

This means you can create empty invalid objects. I would always like my object to be created in a valid state.

You can return the title and author by reference to prevent a copy operation when it is not needed. Just make sure it is a const reference.

    std::string titleIs() const { return title_; }
    std::string authorIs() const { return author_; }

Try this:

    std::string const& titleIs()  const { return title_; }
    std::string const& authorIs() const { return author_; }

But I am not convinced that you need any of these get methods. You should analyze your reason for providing accessor methods. The only reason I can see for giving accesses here is to print the book. But that is not a good reason to give accesses. If your use case is printing then write a method that prints the book. Then when printing you will always have the book printed in a consistent way.

Is there really a user story that allows a books title or author be changed? I can't imagine a book changing its Author once created!

    void updateTitle( const std::string& title ) { title_ = title; }
    void updateAuthor( const std::string& author ) { author_ = author; }
    void updateYear( unsigned int year ) { year_ = year; }

There is a use case for over-writing an object. But this is covered by the assignment operator. So you should define this rather than individual set methods.

Personally I would make these friend methods. They are tightly coupled to the definition of a book so why not make that contract explicit. It also prevents you from declaring all these un-needed get methods.

bool operator==( const Book& first, const Book& other );
bool operator!=( const Book& first, const Book& other );
std::ostream& operator<<( std::ostream& out, const Book& book );

Don't write the same function in two different ways:

bool operator!=( const Book& first, const Book& other ) {
    return ( first.titleIs()  != other.titleIs()  ||
             first.authorIs() != other.authorIs() ||
             first.yearPublished() != other.yearPublished() );
}

Take note that operator== and operator!= are basically the same function. The difference is that one is the inverse of the other. So writting the same function twice opens up your code to subtle errors where one function is changed but the other is not changed in the same way. So rather than two function write one in terms of the other:

bool operator!=(Book const& lhs, Book const& rhs) { return !(lhs == rhs);}

This is strange.

    typedef const std::unordered_multimap<std::string, Book> LibraryMapConst;

Do you have an object of type: LibraryMapConst in your code.

I would note that there is a subtle difference between:

    LibraryMapConst::const_iterator
    LibraryMap::const_iterator

I would have defined a local version of iterator and const_iterator not a LibraryMapConst.

Don't need to declare this:

    Library() = default;

You don't define another constructor so this one will be defined by default. You don't need to be explicit.

Nice to add a book by reference (though it should preferably be const as this allows temporary object to be passed.

    void addBook( const std::string& id, Book& book );

But you may also want to add a version that allows a move:

    void addBook( const std::string& id, Book const& book ); // Copy
    void addBook( const std::string& id, Book&& book );      // Move

Here is a bug:

Book* Library::findBook( const std::string& id ) {
    auto it = books_.find( id );
    return &it->second;
}

If find() does not find a book then the iterator returned is end(). Accessing second from the end() iterator is undefined behavior.

That is not the correct type.

    LibraryMapConst::const_iterator it = books_.begin();

Subtle difference in type here. Glad it worked.

This would have been the correct way in C++03.
But in C++11 we added the range based for that makes this loop easier.

    LibraryMapConst::const_iterator it = books_.begin();
    for( ; it != books_.end(); ++it ) {
        std::cout << "ID " << it->first << '\n'
            << it->second << std::endl;
    }

Now it is easier to write like this:

    for(auto const& book: books_) {
        std::cout << "ID " << book.first << '\n'
            << book.second << '\n'; // prefer \n over std::endl
                                    // that extra flush is very inefficient.
    }

Answer 3

Martin’s review was pretty thorough, so I don’t need to repeat that. A few points to add, though:

typedef std::unordered_multimap<std::string, Book> LibraryMap
typedef const std::unordered_multimap<std::string, Book> LibraryMapConst;

You would not make two separate aliases for regular and const versions. Seeing const LibraryMap is just as short and clearer that this is all it is.

And today we would not use typedef, but write it as:

using LibraryMap = std::unordered_multimap<std::string, Book>;

It doesn’t make sense to have multiple books with the same id. You should define the book ID as being unique! So drop the multi.

---

LibraryMapConst::const_iterator it = books_.begin();
for( ; it != books_.end(); ++it ) {
    std::cout << "ID " << it->first << '\n'
        << it->second << std::endl;
}

Why did you write the first clause of the for on the previous line as a separate statement?

IAC, today you would write:

for (const auto& [id, book] : books_)
{
    std::cout << "ID " << id << '\n'
        << book << '\n';
}

(oh, and don’t use std::endl).

---

Book* Library::findBook( const std::string& id ) {
    auto it = books_.find( id );
    return &it->second;
}

So what happens if that id is not found?

---

I see you have a better understanding of std containers now. Keep it up!