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
int
s 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 int
s.
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.
C++
is correct and very close to that of modernC++
. Once this code is correct and presentable. I would then take this and design the same exact thing inC
. I would then display them in parallel. This is just to make sure that theC++
is well written and well formed for its purpose. \$\endgroup\$std::setw( 15 )
15 is a magic number. \$\endgroup\$