Revised console-based quiz application

Question

This question is a follow up to my previous question, link to the post can be found here. I really appreciate the reviewers especially @Edward's review. They highlighted great points which were considered whilst refactoring the codebase.

OVERVIEW

This project is a console-based quiz application. It can be constructed from a xml file and a std::vector of quiz objects. Quiz Application was designed as one of the ways in which users of the class Quiz might program a quiz application. This insinuates that Quiz Application might be programmed in another way depending on the taste of the user.

AIM

1. I aim to practice object-oriented programming.
2. I aim to improve my ability to produce robust and usable classes.
3. I aim to solidify my knowledge on STL containers and generic algorithms.

MAJOR CONCERN

I don't know if this fits for code review site or maybe should be posted on a different stack exchange site. I planned on persisting objects of type QuizApplication. SOLID design principle requires that classes do one thing and do it very well. QuizApplication so far, generates quiz and grades quiz, does it make any sense for it to also persist an object of its type or should the job be moved to a different class, keeping in mind that the storage facility might change in the near future?

Quiz.h

#ifndef QUIZ_H_
#define QUIZ_H_
#include <iostream>
#include <vector>
#include <string>
#include <map>
struct Quiz 
{
 friend std::ostream& operator<<( std::ostream &os, const Quiz &quiz );
 Quiz();
 Quiz( const std::string &question, const std::vector<std::string>& options, \
 const std::string& answer, const std::vector<std::string>& tag );
 bool operator==( const Quiz& quiz );
 bool operator==( const Quiz& quiz ) const;
 std::string question_;
 std::string answer_;
 std::vector<std::string> tag_;
 std::map<char, std::string, std::less<char>> options_;
};
#endif

Quiz.cpp

#include "Quiz.h"
#include <iostream>
Quiz::Quiz( const std::string &question, const std::vector<std::string>& options, \
const std::string& answer, const std::vector<std::string>& tag )
 : question_{ question }, answer_{ answer }, tag_{ tag } 
 {
 int alphabelt = 65;
 for( const auto item : options )
 options_.insert( { char( alphabelt++ ) , item } );
 } 
std::ostream& operator<<( std::ostream &os, const Quiz &quiz )
{
 for( const auto &item : quiz.tag_ )
 os << "[" << item << "]"; 
 os << '\n' << quiz.question_ << '\n';
 for( const auto item : quiz.options_ )
 os << item.first << ". " << item.second << '\n';
 return os;
}
bool Quiz::operator==( const Quiz& quiz )
{
 if( question_ != quiz.question_ ) return false;
 if( answer_ != quiz.answer_ ) return false;
 if( options_ != quiz.options_ ) return false;
 if( tag_ != quiz.tag_ ) return false;
 return true;
}
bool Quiz::operator==( const Quiz& quiz ) const
{
 if( question_ != quiz.question_ ) return false;
 if( answer_ != quiz.answer_ ) return false;
 if( options_ != quiz.options_ ) return false;
 if( tag_ != quiz.tag_ ) return false;
 return true;
}

QuizApplication.h

#ifndef QUIZAPPLICATION_H_
#define QUIZAPPLICATION_H_
#include "Quiz.h"
#include <iostream>
#include <vector>
#include <unordered_set>
#include <map>
struct QuizHash
{
 size_t operator()( const Quiz &quiz ) const
 {
 return std::hash<std::string>()( quiz.question_ );
 }
};
class QuizApplication
{
 friend std::ostream& operator<<( std::ostream &os, const QuizApplication& app );
 public:
 QuizApplication( const std::vector<Quiz>& quiz )
 : quiz_container_{ quiz } {}
 explicit QuizApplication( const std::string &filename ) { read_xml( filename); };
 QuizApplication& add_quiz( const Quiz& quiz );
 QuizApplication& remove_quiz( const Quiz& quiz );
 void start_quiz() const;
 std::vector<Quiz>::iterator get( Quiz& quiz );
 std::vector<Quiz>::const_iterator get( const Quiz& quiz ) const;
 static int number_of_quiz_to_ask;
 private:
 std::vector<Quiz> quiz_container_;
 void read_xml( const std::string& filename );
 std::unordered_set<Quiz, QuizHash> generate_quiz_set() const;
 unsigned grade_quiz( std::multimap<char, Quiz> &quiz_choice ) const;
};
#endif

QuizApplication.cpp

#include "QuizApplication.h"
#include <iostream>
#include <algorithm>
#include <random>
#include <map>
#include <boost/property_tree/ptree.hpp>
#include <boost/property_tree/xml_parser.hpp>
int QuizApplication::number_of_quiz_to_ask = 0;
void QuizApplication::read_xml( const std::string& filename )
{
 namespace pt = boost::property_tree;
 pt::ptree tree;
 pt::xml_parser::read_xml( filename, tree );
 Quiz quiz_obj 
 { "",
 {},
 "",
 {}
 };
 
 for( auto &quiz_item : tree.get_child( "quizlist" ) )
 { 
 int alphabelt = 65;
 if( quiz_item.first == "quiz" )
 {
 quiz_obj.question_ = quiz_item.second.get<std::string>("question");
 quiz_obj.answer_ = quiz_item.second.get<std::string>("answer");
 
 auto option_list = quiz_item.second.get_child("optionlist");
 for( auto &item : option_list )
 {
 quiz_obj.options_.insert( { char( alphabelt++ ), item.second.data() } );
 }
 auto tag_list = quiz_item.second.get_child("taglist");
 for( auto &item : tag_list )
 {
 quiz_obj.tag_.push_back( item.second.data() );
 }
 }
 quiz_container_.push_back( quiz_obj );
 quiz_obj.options_ = {};
 quiz_obj.tag_ = {};
 }
}
QuizApplication& QuizApplication::add_quiz( const Quiz& quiz )
{
 quiz_container_.push_back( quiz );
 return *this;
}
QuizApplication& QuizApplication::remove_quiz( const Quiz& quiz )
{ 
 auto iter = get( quiz );
 if( iter != quiz_container_.cend() )
 quiz_container_.erase( iter );
 return *this;
}
std::vector<Quiz>::iterator QuizApplication::get( Quiz& quiz )
{
 auto iter = std::find_if( quiz_container_.begin(), quiz_container_.end(), \
 [quiz]( Quiz& this_quiz ) { return ( this_quiz == quiz ); } );
 return iter;
};
std::vector<Quiz>::const_iterator QuizApplication::get( const Quiz& quiz ) const
{
 auto iter = std::find_if( quiz_container_.cbegin(), quiz_container_.cend(), \
 [quiz]( const Quiz& this_quiz ) { return ( this_quiz == quiz ); } );
 return iter;
}
std::ostream& operator<<( std::ostream &os, const QuizApplication& app )
{
 for( const auto& item : app.quiz_container_ )
 os << item << '\n';
 return os;
}
void QuizApplication::start_quiz() const
{
 std::unordered_set<Quiz, QuizHash> quiz_set = generate_quiz_set();
 std::multimap<char, Quiz> quiz_choice;
 for( const auto &item : quiz_set )
 {
 std::cout << '\n' << item << "? ";
 char choice;
 std::cin >> choice;
 quiz_choice.insert( { toupper( choice), item } );
 }
 unsigned score = grade_quiz( quiz_choice );
 char choice;
 std::cout << "Do you want your grade? (y/n)";
 std::cin >> choice;
 if( choice == 'y' ) std::cout << "Your score is " << score << std::endl;
 else std::cout << "See you some other time" << std::endl;
}
std::unordered_set<Quiz, QuizHash> QuizApplication::generate_quiz_set() const
{
 std::unordered_set<Quiz, QuizHash> quiz_set;
 std::default_random_engine engine( static_cast<unsigned>( time( nullptr) ) );
 std::uniform_int_distribution<unsigned> random_int( 0, quiz_container_.size() - 1 );
 for( int i = 0; i != number_of_quiz_to_ask; )
 {
 unsigned index = random_int( engine );
 auto insert_return = quiz_set.insert( quiz_container_[ index ] );
 if( insert_return.second )
 ++i;
 else 
 continue;
 }
 return quiz_set;
}
unsigned QuizApplication::grade_quiz( std::multimap<char, Quiz> &quiz_choice ) const
{
 unsigned score = 0;
 for( const auto& item : quiz_choice )
 {
 if( item.second.options_.at( item.first ) == item.second.answer_ ) 
 ++score;
 }
 return score;
}

quiz.xml

<quizlist>
 <quiz>
 <question>Pointer-based code is complex and error-prone—the slightest omissions or oversights
can lead to serious memory-access violations and memory-leak errors that the compiler
will warn you about.</question>
 <optionlist>
 <option>True</option>
 <option>False</option>
 </optionlist>
 <answer>False</answer>
 <taglist>
 <tag>C++</tag>
 <tag>Programming</tag>
 </taglist>
 </quiz>
 <quiz>
 <question>deques offer rapid insertions and deletions at front or back and direct access to any element.</question>
 <optionlist>
 <option>False</option>
 <option>True</option>
 </optionlist>
 <answer>True</answer>
 <taglist>
 <tag>C++</tag>
 <tag>Programming</tag>
 </taglist>
 </quiz>
</quizlist>

main.cpp

#include <iostream>
#include "Quiz.h"
#include "QuizApplication.h"
int main()
{
 QuizApplication::number_of_quiz_to_ask = 2;
 Quiz quiz_obj
 {
 "What is my name?",
 { "Samuel", "Kingsley", "Paul", "John" },
 "Samuel",
 { "Name" }
 };
 QuizApplication app { "quiz.xml" };
 app.add_quiz( quiz_obj );
 app.start_quiz();
}

Answer 1

Quiz class:

class Quiz - I would probably call this class Question instead. In English a "quiz" would be the entire set of questions.

---

Quiz();

It looks like the default constructor is declared, but not defined. We should do Quiz() { } or Quiz() = default;

---

Quiz( const std::string &question, const std::vector<std::string>& options, \
 const std::string& answer, const std::vector<std::string>& tag );

It's fine to split function definitions across multiple lines. There's no need for the \.

---

bool operator==( const Quiz& quiz );
bool operator==( const Quiz& quiz ) const;

We don't need both of these. A const member function can be called on both a const and non-const Quiz object, so we only need the const version.

Alternatively, we could make this a free function that takes two Quiz objects:

bool operator==(const Quiz& a, const Quiz& b);

---

We store the answer string twice in Quiz - once in the answer_ variable, and once in the options_ map. We only need to store the char key of the correct answer in the answer_ variable.

---

 int alphabelt = 65;
 for( const auto item : options )
 options_.insert( { char( alphabelt++ ) , item } );

Typo: in English it's alphabet.

We could avoid the cast from int to char and make it a little clearer by doing: char alphabet = 'A';. char is an integer type too, so we can increment it.

We probably want to use const auto& item in the range-based for loop, instead of const auto item to avoid an unnecessary copy.

---

Since we're using alphabet letters to refer to questions, we should check somewhere that we don't have more questions in a quiz than characters in the alphabet.

---

QuizHash class:

Using a hash of the entire question string as a storage key is probably not a good idea. (It's going to be slow to compute the hash value, and we have to know the entire question string to find a question).

I don't think the set is actually necessary here, we could just use a vector.

---

QuizApplication class:

 std::vector<Quiz>::iterator get( Quiz& quiz );
 std::vector<Quiz>::const_iterator get( const Quiz& quiz ) const;

These functions are only needed inside the QuizApplication (and outside code has no way to access the quiz_container_) so they could be private.

---

 static int number_of_quiz_to_ask;

number_of_questions_to_ask would be a more accurate name.

---

Quiz quiz_obj 
{ "",
 {},
 "",
 {}
};

We could use the default constructor (assuming it's defined - see above): Quiz quiz_obj;

---

 int alphabelt = 65;

Same as in Quiz class (alphabet, use a char - see above).

---

 quiz_obj.options_ = {};
 quiz_obj.tag_ = {};

We could avoid having to reset the variable every loop iteration by declaring quiz_obj inside the loop.

---

auto iter = std::find_if( quiz_container_.begin(), quiz_container_.end(), \
 [quiz]( Quiz& this_quiz ) { return ( this_quiz == quiz ); } );
return iter;

We have an operator== for Quiz, so we should able to use std::find(quiz_container_.begin(), quiz_container_.end(), quiz); instead.

Again, there's no need for the \.

---

Generating the quiz set

std::unordered_set<Quiz, QuizHash> QuizApplication::generate_quiz_set() const

We don't want to duplicate all the Quiz data when generating the question set. We can generate a set of random indices into quiz_container_ instead, something like:

std::vector<std::size_t> QuizApplication::generate_quiz_set() const
{
 std::vector<std::size_t> out(quiz_container.size(), 0);
 std::iota(out.begin(), out.end(), 0); // out = { 0, 1, 2, 3 ... quiz_container.size() - 1 }
 
 std::default_random_engine engine(static_cast<unsigned>(time(nullptr)));
 std::shuffle(out.begin(), out.end(), engine); // shuffle the indices
 
 out.resize(std::min(number_of_quiz_to_ask, out.size())); // chop off the ones we don't need
 
 return out;
}

---

Running the quiz:

void QuizApplication::start_quiz() const
{
 std::unordered_set<Quiz, QuizHash> quiz_set = generate_quiz_set();
 std::multimap<char, Quiz> quiz_choice;
 for( const auto &item : quiz_set )
 {
 std::cout << '\n' << item << "? ";
 char choice;
 std::cin >> choice;
 quiz_choice.insert( { toupper( choice), item } );
 }
 ...

We should check that the choice character is a valid option for the question, and ask the user again if it isn't. Currently, we'll throw an exception when grading the quiz later.

Again, I don't think we need the set or the multimap. If we use a vector to store the generated quiz set, we could simply use another vector to store the choices:

void QuizApplication::start_quiz() const
{
 std::vector<std::size_t> quiz_set = generate_quiz_set();
 std::vector<char> quiz_choices;
 
 for( const auto &item : quiz_set )
 {
 std::cout << '\n' << item << "? ";
 char choice;
 std::cin >> choice;
 // todo: check choice is a valid option!!!
 quiz_choices.push_back(choice);
 }
 
 ...

Now quiz_choices[i] contains the response to quiz_set[i], where the value stored in quiz_set[i] is the index to the Quiz in quiz_container_. So now marking the quiz looks something like:

unsigned score = 0;
for (auto i = std::size_t{ 0 }; i != quiz_choices.size(); ++i)
{
 auto const& choice = quiz_choices[i];
 auto const& index = quiz_set[i];
 
 if (choice == quiz_container_[index].answer_) // assuming we change answer_ to be a `char` instead of the full string
 ++score;
}

---

• theProgrammer: Good points. I actually preferred a set to a vector cause I want to generate only unique questions. Secondly, if I consider renaming Quiz to Question, then it would make no sense to add data member answer to it.
• theProgrammer: Moreso, a user might mistakenly add a quiz object twice into the container, if I had used indices for generating the question set, their is a chance of having non-unique questions.
• pacmaninbw: @theProgrammer The renaming of Quiz to Question and QuizApplication to Quiz seems to be in line with some of the answers from the previous version of the question.