trjhtr

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 libraryn";
 std::cout << "2: Remove book from the libraryn";
 std::cout << "3: Display the number of books in the libraryn";
 std::cout << "4: Display a books informationn";
 std::cout << "5: Display the list of all booksn";
 std::cout << "6: Display menu optionn";
 std::cout << "7: Exit the library and quitn";
 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' 

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' 

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;

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.

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.