8
\$\begingroup\$

I'm quite new to programming and was hoping you wouldn't mind taking a look at my code and giving some points as to how I could clean it up.

Assignment guidelines:

Create a program that reads multiple choice questions from 1 file and gives 10 random questions to the user. Do Not uses classes.

Example of how the questions are stored in the text document I created

  • Question (end of line)
  • Option1 (end of line)
  • Option2 (end of line)
  • Option3 (end of line)
  • Option4 (end of line)
  • Answer (end of line)
  • (intentionally left blank)
  • Question2...Question20 (rinse and repeat for remaining questions)

Code I used:

#include <iostream>
#include <fstream>
#include <cstring>
#include <iomanip>

using namespace std;

struct questionFormat
{
    string question;
    string answersA;
    string answersB;
    string answersC;
    string answersD;
    string answerSheet;
    string space;
};

int main ()
{
    questionFormat q;
    string questions [20];
    string answersA [20];
    string answersB [20];
    string answersC [20];
    string answersD [20];
    string answerSheet [20];
    string space[20];
    int userAnswer [10];
    int testQuestions [10] = {99,99,99,99,99,99,99,99,99,99};
    int counter = 0, num = 0, userCorrect =0, correctAnswer = 0;
    fstream infile("testQuestions.txt", ios::in);
    srand(time(NULL));

    do
    {
        getline (infile, q.question);
        questions[counter] = q.question;    

        getline (infile, q.answersA);
        answersA[counter] = q.answersA;

        getline (infile, q.answersB);
        answersB[counter] = q.answersB;

        getline (infile, q.answersC);
        answersC[counter] = q.answersC; 

        getline (infile, q.answersD);   
        answersD[counter] = q.answersD;

        getline (infile, q.answerSheet);
        answerSheet[counter] = q.answerSheet;

        getline (infile, q.space);
        space[counter] = q.space;

        counter++;
    } while (counter < 20);

    cout << "Welcome to my fantastic test.\nYou will randomly be given 10/20 questions." << endl;

    counter = 0;

    while (counter < 10)
    {
        num = rand()%20;
        cout << "Current random number: " << num << endl;
        cout << "Counter is: " << counter << endl;
        if (bool exists = find(begin(testQuestions), end(testQuestions), num) != end(testQuestions))
        {
            continue;
        }
        else
        {
            testQuestions[counter] = num;
            cout << "\nQuestion # " << counter +1 << endl;    
            cout << questions [num] << endl;
            cout << answersA [num] << endl;
            cout << answersB [num] << endl;
            cout << answersC [num] << endl;
            cout << answersD [num] << endl;
            cin >> userAnswer[counter];

            correctAnswer = stoi(answerSheet[num]);
            if (userAnswer[counter] == correctAnswer)
            {
                userCorrect++;
            }
            counter++;
        }
    }
    cout << "You got a total of " << userCorrect << " out of 10 correct" << endl;

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ As far as I can see your task is just to ask 10 questions — it does not require reading the user's asnwers, let alone verifying them... ;-) \$\endgroup\$ – CiaPan Sep 12 '16 at 10:46
  • \$\begingroup\$ BTW, 'asking 10 question' usualy means 'asking 10 different questions'. You might want to improve your solution by taking some measures against asking the same question again. \$\endgroup\$ – CiaPan Sep 12 '16 at 10:52
  • \$\begingroup\$ Each time the program is run, it should ask 10 different questions. The find should check to see if that number has already been used and if so, pull a different random number, it'll keep looping until if finds a number that hasn't been used and then proceed. \$\endgroup\$ – Zardoz Sep 25 '16 at 5:34
7
\$\begingroup\$

Cleanup questionFormat

  1. You don't need the space variable.
  2. The variables answersA, etc. should be answerA, etc.
  3. The type of answerSheet can be int. I also think that correctAnswer is a better name for the member.
struct questionFormat
{
   string question;
   string answerA;
   string answerB;
   string answerC;
   string answerD;
   int correctAnswer;
};

Use an array of questionFormat instead of many arrays of strings

The lines

string questions [20];
string answersA [20];
string answersB [20];
string answersC [20];
string answersD [20];
string answerSheet [20];
string space[20];

can be replaced by

questionFormat questions[20];

Create function for reading data

Create function readData and move the code to read the data from main to readData.

int readData(std::string const& inputFile,
              questionFormat questions[],
              int maxQuestions)
{
   fstream infile("testQuestions.txt", ios::in);
   ...
}

Don't assume file IO succeeded

Always check status of file IO operations. It's never safe to assume that they succeeded.

Change the lines that read the input to:

for (counter = 0; counter < maxQuestions; ++counter )
{
   questionFormat& q = questions[counter];

   if ( !getline (infile, q.question) )
   {
      std::cout << "Failed to read a question.\n";
      return counter;
   }

   if ( !getline (infile, q.answerA) )
   {
      std::cout << "Failed to read answer A to a question.\n";
      return counter;
   }

   if ( !getline (infile, q.answerB)  )
   {
      std::cout << "Failed to read answer B to a question.\n";
      return counter;
   }

   if ( !getline (infile, q.answerC) )
   {
      std::cout << "Failed to read answer C to a question.\n";
      return counter;
   }

   if ( !getline (infile, q.answerD) )
   {
      std::cout << "Failed to read answer D to a question.\n";
      return counter;
   }

   if ( !(infile >> q.correctAnswer ) )
   {
      std::cout << "Failed to read the correct answer to a question.\n";
      return;
   }

   // Ignore two lines of text.
   // The first line corresponds to what's left in the stream
   // after reading the correct answer.
   // The second line is the intentionally left space.
   infile.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
   infile.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}

Simplify the find line

The line

if (bool exists = find(begin(testQuestions), end(testQuestions), num) != end(testQuestions))

can be simplified to:

if (find(begin(testQuestions), end(testQuestions), num) != end(testQuestions))

Move the code to conduct the tests to a separate function

void conductTests(questionFormat questions[],
                  int maxQuestions)
{
   ...
}

The contents of conductTests will be different from your posted code due to use of an array of questionFormat to hold the data.

That will simplify main to:

int main ()
{
   questionFormat questions[20];

   srand(time(NULL));

   int numQuestions = readData("testQuestions.txt", questions, 20);

   conductTests(questions, numQuestions);

   return 0;
}

No need to use an array for userAnswer

The value is read and used inside every iteration of the loop. It can be a simple int. Not only that, it can be in the scope of the while loop instead of being in the function scope.


Here's the complete program:

#include <iostream>
#include <fstream>
#include <cstring>
#include <iomanip>
#include <string>
#include <cstdlib>
#include <limits>
#include <algorithm>

using namespace std;

struct questionFormat
{
   string question;
   string answerA;
   string answerB;
   string answerC;
   string answerD;
   int correctAnswer;
};

int readData(std::string const& inputFile,
             questionFormat questions[],
             int maxQuestions)
{
   fstream infile("testQuestions.txt", ios::in);

   for (int counter = 0; counter < maxQuestions; ++counter )
   {
      questionFormat& q = questions[counter];

      if ( !getline (infile, q.question) )
      {
         std::cout << "Failed to read a question.\n";
         return counter;
      }

      if ( !getline (infile, q.answerA) )
      {
         std::cout << "Failed to read answer A to a question.\n";
         return counter;
      }

      if ( !getline (infile, q.answerB)  )
      {
         std::cout << "Failed to read answer B to a question.\n";
         return counter;
      }

      if ( !getline (infile, q.answerC) )
      {
         std::cout << "Failed to read answer C to a question.\n";
         return counter;
      }

      if ( !getline (infile, q.answerD) )
      {
         std::cout << "Failed to read answer D to a question.\n";
         return counter;
      }

      // Ignore two lines of text.
      // The first line corresponds to what's left in the stream
      // after reading the correct answer.
      // The second line is the intentionally left space.
      infile.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
      infile.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
   }

   return maxQuestions;
}

void conductTests(questionFormat questions[],
                  int maxQuestions)
{
   int testQuestions [10] = {99,99,99,99,99,99,99,99,99,99};
   int counter = 0;
   int userCorrect = 0;

   cout << "Welcome to my fantastic test.\nYou will randomly be given 10/20 questions." << endl;

   while (counter < 10)
   {
      int userAnswer;
      int num = rand()%maxQuestions;
      cout << "Current random number: " << num << endl;
      cout << "Counter is: " << counter << endl;
      if (find(begin(testQuestions), end(testQuestions), num) != end(testQuestions))
      {
         continue;
      }
      else
      {
         testQuestions[counter] = num;
         cout << "\nQuestion # " << counter +1 << endl;    
         cout << questions [num].question << endl;
         cout << questions [num].answerA << endl;
         cout << questions [num].answerB << endl;
         cout << questions [num].answerC << endl;
         cout << questions [num].answerD << endl;
         cin >> userAnswer;

         if (userAnswer == questions[num].correctAnswer)
         {
            userCorrect++;
         }
         counter++;
      }
   }

   cout << "You got a total of " << userCorrect << " out of 10 correct" << endl;
}

int main ()
{
   questionFormat questions[20];

   srand(time(NULL));

   int numQuestions readData("testQuestions.txt", questions, 20);

   conductTests(questions, numQuestions);

   return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the info. Definitely a lot cleaner and easier to follow. \$\endgroup\$ – Zardoz Sep 12 '16 at 6:08
  • 1
    \$\begingroup\$ I really dont like that you readData leaves memory uninitialized and doesnt even tell the user seomthing went wrong. \$\endgroup\$ – miscco Sep 12 '16 at 7:47
  • \$\begingroup\$ You copy+paste a bit too fast. Please note a plural's '-s' suffix: in struct questionFormat there are either four answers in a group (say an array string answers[4]) or string answerA; string answerB; string answerC; string answerD;In the first case the name 'answers' for the whole group is appropriate, in the second one each answer is stored on its own, so variables should have names in singular. \$\endgroup\$ – CiaPan Sep 12 '16 at 11:22
  • \$\begingroup\$ @miscco, good observation. Updated readData and main to account for less than expected data in the input file. \$\endgroup\$ – R Sahu Sep 12 '16 at 15:09
  • \$\begingroup\$ @CiaPan, agree with your observation. Updated the answer to reflect that. \$\endgroup\$ – R Sahu Sep 12 '16 at 15:12
5
\$\begingroup\$

As @phrancis mentioned in the comments, welcome to CR!


Each file should be self-sufficient by including the specific header a symbol relies on. You are missing the following:

#include <algorithm>  // std::find
#include <iterator>   // std::begin, std::end
#include <cstdlib>    // srand, rand
#include <ctime>      // time
#include <string>     // std::string, std::getline, std::stoi

Don't abuse using namespace std;.

Prefer <random> instead of rand()

Prefer '\n' to std::endl unless you want to flush.

Be aware of the differences between pre-increment (++i) and post-increment (i++). Post-increment returns a copy of the current element before increment. Pre-increment increments and returns itself. Prefer pre-increment when you don't need the copy of the previous value as it is semantically correct.


struct questionFormat {
    // ...
};

questionFormat is a record type, formally known as a plain old data struct (POD struct). A record is considered to be a class type by the C++ Standard (§9). The assignment guideline states "Do not use classes", so ensure you are meeting the requirements of the problem ask for clarification if there is any ambiguity.


string questions [20];

Avoid magic constants. Prefer symbolic constants as they provide context and semantics for the literal.

constexpr int number_of_questions = 20;
string questions[number_of_questions];

Be consistent. Some variables are attached to the bracket, some are not. Some function names are attached to the parenthesis', some are not. Use spaces sparingly.

string questions[20];        // no space between variable & array brackets
getline(infile, q.question); // no space between funcname & parameter braces

int counter = 0, num = 0, userCorrect =0, correctAnswer = 0;

Declare one name per declaration to avoid mistakes related to the language grammar. Helps with readability.

Keep the scope for variables as minimal as possible to avoid accidental reuse, reduces variable tracking (improving readability), and possibly reduces resource retention.

When using an obvious local loop variables, prefer for statements. counter is never used outside of loop constructs and the loops are not dependent on a previous value.

for (int counter = 0; counter < number_of_questions; ++counter) {
    // use counter
}

fstream infile("testQuestions.txt", ios::in);

Make sure the resource was actually acquired.

std::fstream infile(infile_name, ios::in);

if (!infile) {
    // Do something to indicate failure
}

    getline (infile, q.question);
    questions[counter] = q.question;    

Make sure std::getline actually read the value. Don't commit the read-in values until you've ensured all values were read correctly.

    if (!std::getline(infile, q.question)) {
        // handle the failure
    }
    // ...
    if (!std::getline (infile, q.space)) {
        // handle the failure
    }

    // all fields of the record have been successfully read, write them all.
    questions[counter] = q.question;
    // ...

    if (some_condition) {
        continue;
    } else {
        // do something
    }

Avoid else after interrupts in flow control (return, break, continue). You help readability by reducing indentation and reducing the amount of variables/states that need to be tracked.

for (;;) // just some loop
{
    if (some_condition) {
        continue;  // interrupts flow so won't execute rest of the loop body
    }
    // rest of loop
}

if (bool exists = find(begin(testQuestions), end(testQuestions), num) != end(testQuestions))

You don't need to search the entire range. Since you are logging the index as you go, you only need to search from 0 up to the current index (non-inclusive).

Since you never use exists, you can omit it and evaluate the temporary evaluated from the expression.

auto first = std::begin(testQuestions);
auto last = first + num;
if (std::find(first, last, num) != last)
\$\endgroup\$
4
\$\begingroup\$

@RSahu already had some points which I would like to expand.

  1. Do not use namespace std
  2. You should not simply return on an error during I/O, as that would leave your questionFormat structure only partially initialized and most likely lead to some invalid memory access. Instead you should throw an exception.

    if ( !(infile >> q.correctAnswer )) {
        throw std::runtime_error("Cannot read correct Answer!");
    }
    
  3. As you are using C++ you should use stl containers. In that case std::vector should be best suited for you.

    struct questionFormat {
        string question;
        std::string answersA;
        std::string answersB;
        std::string answersC;
        std::string answersD;
        int correctAnswer;
    };
    
    std::vector<questionFormat> questions;
    
  4. C++ has a random library that you might want to think about. It might be considered overkill for your trivial example, but it doesnt hurt to do it right anyway.

  5. Personally i would never use while loops when i know the number of iterations. so instead of

    while (counter < 20)
    

    I would strongly emphasize a for loop

    for (unsigned counter=0; counter < numberQuestionsAsked; ++counter)
    
  6. I would suggest to separate the logic of selecting a test and performing the test.

    std::set<unsigned> selectTests (int numberOfQuestions, int numberQuestionsAsked) {
    std::default_random_engine generator;
    std::uniform_int_distribution<int> distribution(0,numberOfQuestions-1);
    
    std::set<unsigned> choosenQuestions;
    while (choosenQuestions.size() < numberQuestionsAsked) {
        choosenQuestions.insert(distribution(generator));
    }
    return choosenQuestions;
    

    Then in you conductTests function you can do

    for (const auto& index : choosenQuestions) {
        cout << questions.at(index).question << endl;
        cout << questions.at(index).answersA << endl;
        cout << questions.at(index).answersB << endl;
        cout << questions.at(index).answersC << endl;
        cout << questions.at(index).answersD << endl;
        cin >> userAnswer;
        if (userAnswer == questions.at(index).correctAnswer) {
            userCorrect++;
        }
    }
    
  7. Finally consider creating a class with the respective private and public methods. That would help you encapsulate the data better.

\$\endgroup\$

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.