Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird" "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

fixed typo
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

In several cases, a passed parameter is an std::ifstream but the interface would be more flexible if they instead used a std::istream. I'd also recommend having the functions accept a std::istream & to both avoid making a copy and to have the calling function handle any problems opening files. It also makes it slightly easier to write automated tests because you can use std::sringstreamstringstreams as inputs.

In several cases, a passed parameter is an std::ifstream but the interface would be more flexible if they instead used a std::istream. I'd also recommend having the functions accept a std::istream & to both avoid making a copy and to have the calling function handle any problems opening files. It also makes it slightly easier to write automated tests because you can use std::sringstreams as inputs.

In several cases, a passed parameter is an std::ifstream but the interface would be more flexible if they instead used a std::istream. I'd also recommend having the functions accept a std::istream & to both avoid making a copy and to have the calling function handle any problems opening files. It also makes it slightly easier to write automated tests because you can use std::stringstreams as inputs.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

Here are some things that may help you improve your code.

Use only required #includes

The code has the line:

#include <sstream>

but as far as I can see, nothing from <sstream> is actually used or needed. Only include files that are actually needed.

Consider better naming

The function named InitializeQuizGame is actually rather misleading because it doesn't simply initialize the game, but also runs it to completion. Most of the other names are not bad, but the odd mix of capitalization (as with InitializeQuizGame and load) makes the code a bit harder to read than it should be.

Let the computer do the mathematics

If you decided to make a 20 question quiz or a 50 question quiz, it would be nice if the program would automatically adjust to that without having to recompile the program. Fortunately, it's quite easy to do. Assuming the questions are all weighted the same, and if there are \$n\$ questions, each question is worth \$n/100\$ percent. Since you're reading all of the questions into a std::vector anyway, why not just let the computer perform the mathematics rather than hardcoding the values?

When is a failing grade not a failing grade?

If I see a variable named failingGrade I would expect it to actually represent a failing grade, but surprisingly in this code, if one gets 60% correct (even though s_failingGrade = 60) the message says that I passed the quiz.

Perform input sanitation

The function that reads in the questions does not validate the data. In particular, the correct answer is apparently always supposed to be a, b, c, or d but the data read is not validated to assure that.

Prefer std::istream to std::ifstream

In several cases, a passed parameter is an std::ifstream but the interface would be more flexible if they instead used a std::istream. I'd also recommend having the functions accept a std::istream & to both avoid making a copy and to have the calling function handle any problems opening files. It also makes it slightly easier to write automated tests because you can use std::sringstreams as inputs.

Consider more effective use of objects

The Question class is not a bad start for a quiz program, but the object is a little strange in that its only member function relies on both an externally provided question number and various global variables in order to function. These both suggest to me that it might be better to introduce a Quiz class to encapsulate the vector of Question objects. It might look something like this:

class Quiz {
public:
 class Question {
 public:
 friend std::istream& operator >> (std::istream& is, Question& ques);
 bool ask() const;
 private:
 std::string question_text;
 std::string answer_1;
 std::string answer_2;
 std::string answer_3;
 std::string answer_4;
 char correct_answer;
 };
 Quiz(std::istream &data);
 void operator()() const;
private:
 std::vector<Question> questions;
 static const int s_failingGrade = 60;
 static const char* s_winMessage;
 static const char* s_loseMessage;
 static const char* s_promptAnswer;
};
const char* Quiz::s_winMessage = "Correct!\n";
const char* Quiz::s_loseMessage = "Incorrect, the correct answer was ";
const char* Quiz::s_promptAnswer = "What is your answer?\n";

Usage might look like this:

int main()
{
 std::ifstream in{"quiz_data.txt"};
 if (!in) {
 std::cout << "Error: could not open quiz data file\n";
 return 1;
 }
 Quiz quiz(in); //Load questions from .txt file
 quiz();
}

Avoid hardcoding values

The file names, the failing grade and the various error messages are all hardcoded now. Rather than hardcode those, you could read them from a configuration file, making the program much more flexible and usable at very little additional effort.

Consider shuffling the answers

Right now each question is asked with all of the answers in the same order as they appeared in the input file. It might make for a better quiz if the program shuffled the available answers each time, just as it shuffles the questions.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /