5
\$\begingroup\$

I was trying to answer a question on Stack Overflow found here. I took the idea that the OP was trying to do and I wrote my own program based off of their question. I was trying to provide the user with some tips and suggestions while illustrating some of the similarities and differences between C and C++. My goal was not to write best-practice modern C++ standards using strictly C++11, 14, or 17 per se. I however did use a few features here and there to introduce them to the OP. My code is simple. It is a small console program that will allow the user to add books into a library, remove books by ID or book information, find books if they exist, display a single book's information via ID or book information, and to display the entire library's list of books.

I would like to know if there are any improvements that can be made to my program. As far as I can tell it appears to be bug-free as I debugged nearly every case. I would like to try to keep the structure of the code almost nearly as is for I want it to still have the appearance of almost being C-like yet while trying to show some of the C++ features.

My goal here is that when I post this as a viable answer that it will not be down-voted for any kind of discrepancies and things that might have been overlooked.


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;
        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( 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: {
            removeBook( lib );
            std::cout.flush();
            system( "clear" );
            displayMenu();
            std::cout << "\nYou have removed a book from the 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 ) {
                std::cout << *lib;
            } 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 ) {
    static unsigned int bookId = 0;
    unsigned int year = 0;

    std::string title, author;
    std::cin.ignore();
    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;

    bookId++; // increment our book id so each one is unique TODO: this can be replaced to have same id for multiple books if the books are exact matches.

    Book book( title, author, year );
    lib->addBook( std::to_string( 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::cout << "\nRemove book by ID(I) or by Book(B)\n";
    char choice;
    std::cin >> choice;
    if( choice == 'i' || choice == 'I' ) {
        std::cout << "Enter the books ID ";
        unsigned int id;
        std::cin >> id;
        lib->removeBook( std::to_string( id ) );
    } else if( choice == 'b' || choice == 'B' ) {
        std::cin.ignore();
        std::cout << "What is the title of the book? ";
        std::string title;
        std::getline( std::cin, title );
        std::cout << "Who is the author of the book? ";
        std::string author;
        std::getline( std::cin, author );
        std::cout << "What year was the book published ";
        unsigned int year;
        std::cin >> year;
        Book book( title, author, year );
        lib->removeBook( book );
    } else {
        std::cout << "\nYou entered an invalid selection\n";
    }
}

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::cout << "\nFind book by ID(I) or by Book(B)\n";
    char choice;
    std::cin >> choice;
    if( choice == 'i' || choice == 'I' ) {
        std::cout << "Enter the books ID ";
        unsigned int id;
        std::cin >> id;
        Book* book = lib->findBook( std::to_string( id ) );
        if( book ) {
            std::cout << *book;
        } else {
            std::cout << "\nBook was not found.\n";
        }

    } else if( choice == 'b' || choice == 'B' ) {
        std::cin.ignore();
        std::cout << "What is the title of the book? ";
        std::string title;
        std::getline( std::cin, title );
        std::cout << "Who is the author of the book? ";
        std::string author;
        std::getline( std::cin, author );
        std::cout << "What year was the book published? ";
        unsigned int year;
        std::cin >> year;

        Book bookToFind( title, author, year );
        Book* actualBook = lib->findBook( bookToFind );

        if( actualBook ) {
            std::cout << *actualBook;
        } else {
            std::cout << "\nBook was not found.\n";
        }

    } else {
        std::cout << "\nYou entered an invalid selection\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( const std::string& title, const std::string& author, unsigned int year );

    std::string titleIs() const;
    std::string authorIs() const;
    unsigned int yearPublished() const;

    void updateTitle( const std::string& title );
    void updateAuthor( const std::string& author );
    void updateYear( unsigned int year );

    bool operator==( const Book& other ) const;
};

std::ostream& operator<<( std::ostream& out, const Book& book );

#endif // BOOK_H

Book.cpp

#include "Book.h"

Book::Book() {
} // default

Book::Book( const std::string& title, const std::string& author, unsigned int year ) :
    title_( title ),
    author_( author ),
    year_( year ) {
}

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

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

unsigned int Book::yearPublished() const { 
    return year_; 
}

void Book::updateTitle( const std::string& title ) { 
    title_ = title; 
}

void Book::updateAuthor( const std::string& author ) { 
    author_ = author; 
}

void Book::updateYear( unsigned int year ) { 
    year_ = year; 
}

bool Book::operator==( const Book& other ) const {
    return ( title_  == other.title_  &&
             author_ == other.author_ &&
             year_   == other.year_ );
}

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 <map>

class Library {
private:
    std::multimap<std::string, Book> books_;
    // This is used if the library has several copies of the same book
    // that have the same ID in the multimap above.
    std::map<std::string, unsigned int> inventory_; 

public:
    Library(); // deafault

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

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

    std::size_t totalBooks() const;
    std::size_t totalUniqueBooks() const;

    // Three different ways to return the library back to user
    std::multimap<std::string, Book> mapOfBooks() const;

    // Currently Supports List and Vector 
    template< template < class ... > class Container, class ... Args >
    void listOfBooks( Container<Book, Args...>& c ) const;

private:
    // Helper function to calculate the number of unique books.
    std::size_t calculateUniqueNumberOfBooks() const;
};


template<template<class...> class Container, class...Args>
void Library::listOfBooks( Container<Book, Args...>& c ) const {
    auto it = books_.begin();
    while ( it != books_.end() ) {
        c.emplace_back( it->second );
    }
}

std::ostream& operator<<( std::ostream& out, const Library& library );

void displayLibrary( const Library& library );

#endif // LIBRARY_H

Library.cpp

#include "Library.h"
#include <vector>

Library::Library() {
} // deafault

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

void Library::removeBook( const std::string& id ) {
    auto it = books_.begin();
    while( it != books_.end() ) {
        if( id == it->first ) {
            // found match so remove it
            it = books_.erase( it );
        } else {
            it++;
        }
    }
}

void Library::removeBook( Book& book ) {
    auto it = books_.begin();
    while( it != books_.end() ) {
        if( book == it->second ) {
            // found match so remove it
            it = books_.erase( it );
        } else {
            it++;
        }
    }
}

Book* Library::findBook( const std::string& id ) {
    auto it = books_.begin();
    while( it != books_.end() ) {
        if( id == it->first ) {
            return &it->second;
        } else{
            it++;
        }
    }
    return nullptr;
}

Book* Library::findBook( Book& book ) {
    auto it = books_.begin();
    while( it != books_.end() ) {
        if( book == it->second ) {
            return &it->second;
        } else {
            it++;
        }
    }
    return nullptr;
}

std::multimap<std::string, Book> Library::mapOfBooks() const {
    return books_;
}


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

std::size_t Library::totalUniqueBooks() const {
    //TODO: For now just return total number of books
    return books_.size();
}

std::size_t Library::calculateUniqueNumberOfBooks() const {
    //TODO: For now just return total number of books
    return books_.size();
}

std::ostream& operator<<( std::ostream& out, const Library& library ) {
    for( auto b : library.mapOfBooks() ) {
        out << "ID " << b.first << '\n'
            << b.second;
    }
    return out;
}

void displayLibrary( const Library& library ) {
    std::cout << library;
}
\$\endgroup\$
  • \$\begingroup\$ I wouldn't be moved to downvote, but I am perturbed by using pointers in all the library functions in main.cpp; they should all be references. I am also bemused by the way your setters and constructors all take const& (they should take by-val for moving), while your getters all return by-val (you could return const&). I'm also not keen on not using any algorithms for things like findBook. \$\endgroup\$ – indi May 25 '18 at 0:04
  • \$\begingroup\$ @indi Good advice. I understand about the move semantics and other things as such, but remember that the OP this will be presented to is coming from C and is new to C++. I was only trying to introduce some of the C++ features. \$\endgroup\$ – Francis Cugler May 25 '18 at 0:15
  • \$\begingroup\$ Small suggestion: instead of using many std::cout in main.cpp, you can print multiline string literals. \$\endgroup\$ – esote May 25 '18 at 0:18
  • 1
    \$\begingroup\$ @FrancisCugler Yes, passing by const& is definitely still a legit practice in some situations (functions that just use the data, rather than taking it). What's changed since C++11 is that it's no longer the default thing you should do. Move semantics made by-val the new default, which simplifies a lot of stuff without sacrificing efficiency. Move semantics really changed everything. \$\endgroup\$ – indi May 25 '18 at 0:42
  • 1
    \$\begingroup\$ @FrancisCugler Yeah, but I wouldn't show variadic templates to newbies. Simple templates, definitely, but things like template templates and variadic templates, no. \$\endgroup\$ – indi May 25 '18 at 0:45
6
\$\begingroup\$

One thing I notice right away:

Library::Library() {
} // deafault [sic]

Book::Book() {
} // default

Don’t write an empty default constructor. If the compiler does everything automatically, leave it off completely. If you must mention it, just use =default but that still counts as a declaration and there are times where any declaration disables other built-in generation.


Book::Book( const std::string& title, const std::string& author, unsigned int year ) :
    title_( title ),
    author_( author ),
    year_( year ) {
}

First, put this inline in the class definition. Second, use modern syntax: uniform initialization.

More advanced: The strings are “sink” values, so the most efficient thing is to pass them by value and then move into place. So, you get:

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

All the trivial functions in Book.cpp should just be defined right in the class.


You defined == but not a matching !=. Usually these things are defined as non-members taking two arguments, so handling of the two args (typically involving conversions) is symmetric.


// Three different ways to return the library back to user
std::multimap<std::string, Book> mapOfBooks() const;

Do you really want to copy the entire multimap? It might be fine to return a constant reference; that is common enough that users will not be surprised by the lifetime dependency.

How is this “three ways” ?


// Currently Supports List and Vector 
template< template < class ... > class Container, class ... Args >
void listOfBooks( Container<Book, Args...>& c ) const;

No, don’t do that. Just provide the underlying (const) iterators and the user can use std::copy if that’s what he wanted to do, or whatever without having to copy the collection at all.


template<template<class...> class Container, class...Args>
void Library::listOfBooks( Container<Book, Args...>& c ) const {
    auto it = books_.begin();
    while ( it != books_.end() ) {
        c.emplace_back( it->second );
    }
}

You really don’t have to implement std::copy from scratch here.


void Library::removeBook( const std::string& id ) {
    auto it = books_.begin();
    while( it != books_.end() ) {
        if( id == it->first ) {
            // found match so remove it
            it = books_.erase( it );
        } else {
            it++;
        }
    }
}

Isn’t the library a map with id as the key? Yes, you are comparing it with it->first! Why??????

Use the normal way to find an item in the container, which returns an iterator, which you can then remove.

For the other version, I’d suggest implementing a general-purpose find and then remove can just call that.

But then I see you do have a pair of findBook functions, the first of which should be something the container already does directly without a linear search, and the second duplicates most of the body of removeBook.

You should implement that as a private helper that returns the iterator, so you can then use that to erase etc. Then the public finding function can call that and just return second. That is assuming you don’t want to expose the underlying container’s iterators to the user — but earlier I suggested providing iterators to your library. So rethink the design here.

In general, you are wrapping a standard container instead of just letting the user use a standard container. You just need typedef the specific form of map, and define non-member secondary finds for matching the book rather than the key.


I have to go. I may have more later.

\$\endgroup\$
  • \$\begingroup\$ I had provided it already as an answer but was getting complaints about discrepancies in constness, about having operator << as members, the use of system("clear");, etc... I had removed that post altogether and will make a new one with a better viable version! I appreciate any and all feedback! \$\endgroup\$ – Francis Cugler May 25 '18 at 0:00
  • \$\begingroup\$ As for the default constructor; there may be cases where the object is constructor first then populated with information later, and their may be cases where the object is constructed with the information upon creation, however that only pertains to the Book class and not the Library class. \$\endgroup\$ – Francis Cugler May 25 '18 at 0:02
  • \$\begingroup\$ No problem; thank you for any and all advice. I'll be looking forward to seeing what I can do with your's and other's suggestions. \$\endgroup\$ – Francis Cugler May 25 '18 at 0:20
  • 1
    \$\begingroup\$ I think you misunderstand: not defining the default constructor yourself means that one is automatically generated by the compiler. Using default initializers on the data members tells the compiler how to do that. \$\endgroup\$ – JDługosz May 25 '18 at 8:02
  • \$\begingroup\$ I have this update question: codereview.stackexchange.com/q/195713/130733 \$\endgroup\$ – Francis Cugler Jun 2 '18 at 21:23
3
\$\begingroup\$

I like what you are trying to do, this reminds me of a talk on CppCon: extern c: Talking to C Programmers about C++. With that talk in mind, here is my feedback.

The following elements are shown in your example:

  • Classes
  • Streams
  • Strings
  • References
  • Const
  • Overloading
  • Standard library containers (map, unordered map, pair ...)
  • Templates
  • auto

Looking at this list, I'm wondering if this ain't too overwhelming for a C programmer resulting in an instinctive defensive reflex rejecting all of this. From this point of view, I'd suggest to reduce this list. However, as a C++ programmer, I'd rather see it extended.

Looking at that, I'd wonder which kind of problems you want C++ to be the solution to. I'm gonna pick out some I recognize and find important myself:

  • RAII (std::string class always freeing memory)
  • Simplify code (std::cout / std::cin vs printf / scanf)
  • References to disallow nullptr
  • Grouping code logically
  • Constructors as alternative for initialization
  • Generic code

If I should choose, I would drop templates, auto and streams (including overloading) in favor of the elements which prevent bugs. Once they see the added value they will discover more.

This would bring me to following list:

  • Strings
  • References
  • Const

which I would extend with:

  • unique_ptr (no make_unique needed), can we ignore rvalue references while introducing std::move?

Future

Once they see the added value, you can extend the structs with methods and introduce private members. As well as standard library containers, which will quickly lead you to lambdas when you introduce std::sort.

Than you can add streams as printf on those classes becomes a problem. Overloading and auto can be added at any moment.

Finally I would introduce templates.

\$\endgroup\$
  • \$\begingroup\$ I was doing some research on how to use unordered_multimap efficiently and I came across this YouTube video that sponsored Jason Turner. It is not one of his regular C++ video tutorials that can be found on his YouTube channel, but a presentation at CppCon where he writes Pong in C++17. The code will convert the x86 assembly to the 6502 assembly. He shows how to use many different features of C++17 where the compiler will optimize away a majority of the code with nearly 0 overhead. The code does just what it is supposed to do. Here is the link: youtube.com/watch?v=zBkNBP00wJE \$\endgroup\$ – Francis Cugler May 28 '18 at 15:56
  • 1
    \$\begingroup\$ Great talk, I'm familiar with it. \$\endgroup\$ – JVApen May 28 '18 at 16:09
  • \$\begingroup\$ Continued... The code was then ported over to a Commodore 64 and it ran like it was supposed to. The reason I mentioned the video above is because of one of the comments on that YouTube page mentioned the fact that bringing C people over to C++ is a hard feat if you can not show them how to do 0 overhead. And this video of Jason Turner using modern C++14 & 17... Shows just that! \$\endgroup\$ – Francis Cugler May 28 '18 at 16:11
  • \$\begingroup\$ Oh; it might not of been the 6502, it might of been the 6510... but still it is doing 8 bit math in C++17 while watching the compiler do its job! \$\endgroup\$ – Francis Cugler May 28 '18 at 16:11
  • \$\begingroup\$ I have this updated question: codereview.stackexchange.com/q/195713/130733 \$\endgroup\$ – Francis Cugler Jun 2 '18 at 21:23

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.