C++, Infinite while loop

Question

My C++ project purpose is to output a file with a picture made from simple characters.

The first part of my project requires that I "include a function that read a command from the input file and returns an integer value representing the command that was read". That's my takeCommand function. Seems horribly inefficient, but I guess necessary if, I understood it correctly.

The current problem is that my the while loop in my main function runs an infinite loop. When outputting the text "Works", it constantly repeats. My intention was to make an End-of-File controlled loop. Inside the takeCommand function, when it reads the last line, the takeCommand function ceases to run, thus the while loop ends.

I need ideas as to how to get the while loop to work without repeating constantly.

What I currently have is:

    int takeCommand(int& num, int& cmd);

    ifstream infile;
    ofstream outfile;

    int main()
    {
         const int MAX_FOR = 3;
         string str;
         int num, cmd;
         infile.open("DrawingInput_01.txt");
         outfile.open("DrawingOutput_01.txt");

         for (int i = 0; i < MAX_FOR; i++)
         {
             getline(infile,str);
         }

         takeCommand(num,cmd);
         while(infile)
         {
           cout << "Works";
           /*switch(cmd)
           {
               case '1': printSpace(infile, outfile); break;
               case '2': printChar(infile, outfile); break;
               case '3': printNewline(infile, outfile); break;
               case '0': break;
           }*/
           takeCommand(num, cmd);
         }

         infile.close();
         outfile.close();
         return 0;
     }

     int takeCommand(int& num, int& cmd)
     {
         string str;
         char firstChar;
         infile.open("DrawingInput_01.txt");
         for (int i = 0; i < 3; i++)
         {
             getline(infile.str);
         }
         infile >> firstChar >> str >> num;
         switch(firstChar)
         {
             case 's': cmd = 1; break;
             case 'p': cmd = 2; break;
             case 'n': cmd = 3; break;
             case 'q': cmd = 0; break;
         }
         return cmd;
     }

One of the input files looks like:

; CS 1044 Fall 2010
; Project 4
; Basic Cat
space 1
print 1 /
print 1 \
print 1 _
print 1 /
print 1 \
newline
print 1 (
space 1
print 1 o
print 1 .
print 1 o
space 1
print 1 )
newline
space 1
print 1 >
space 1
print 1 ^
space 1
print 1 <
newline
quit

The output file is supposed to make a bunny.

The void functions are in my code, it's just not the problem right now.

Answer 1

Your problem is that infile is alive throughout the while loop's execution, so your while loop is always true because the object still exists even after the file is closed.

Additionally, you should be getting the inputted string before you test it with the switch statement, and you need to close the file inside the while loop when I/O operations indicate that the file has been totally read (I.e with the eof() function, which must be called after data has been read). And you're opening the file again in you're second function. This code needs serious revision.

Answer 2

Well let's first start with your takeCommand function: You used a looping mechanism and getline to ignore the first three lines in the file (because it was a simple header). Seems logical, but there's a caveat. When you call this function multiple times from within the while loop, it will ignore 3 lines per each call. How's that for "horribly inefficient"?! :)

You don't need to do this. Besides, you initially ignore the first three lines inside the main function (outside the while loop) so why do you need it again inside takeCommand? The solution is to remove the for loop so that your function afterwards looks like this:

int takeCommand(int& num, int& cmd)
{
    char firstChar;
    infile.open("DrawingInput_01.txt");

    infile >> firstChar >> str >> num;

    switch (firstChar)
    {
        case 's': cmd = 1; break;
        case 'p': cmd = 2; break;
        case 'n': cmd = 3; break;
        case 'q': cmd = 0; break;
    }
    return cmd;
}

Another pet-peeve I have is an unnecessary open call. If you already know the name of the file at the time of the construction of the file stream, simply supply it when the file stream is created. That means removing that call from the function and instead doing:

ifstream infile("DrawingInput_01.txt");

And another thing, the file streams don't need static-storage duration, and they are not needed as global variables, so you should take the input and output file streams and put them inside main.

The while loop condition is all wrong. The I/O should be performed before the stream is checked. So you can change your function to:

std::basic_ios<char>& takeCommand(int& cmd)
{
    char firstChar;
    if (!inFile.get(firstChar))
    {
        return inFile;
    }

    inFile.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    // ...

    return inFile;
//  ^^^^^^^^^^^^^^
}

This allows the stream to be returned to the while loop condition below so that it will check if the input succeed or not:

while (takeCommand(cmd))

Notice how I've also checked for a successful input operation (and if there was, we ignore the rest of the line) and that I've taken out the unneeded parameter.

And after you've made these changes (among others), this is what your program should look like:

#include <iostream>
#include <fstream>
#include <limits>

std::ios& takeCommand(std::istream&, int&);

int main()
{
    std::ifstream inFile("DrawingInput_01.txt");
    std::ofstream outFile("DrawingInput_01.txt");
    std::string str;

    for (int i = 0; i < 3; ++i) std::getline(inFile, str);

    int cmd;

    while (takeCommand(inFile, cmd))
    {
        std::cout << "Command was " << cmd;
        outFile << cmd;
   }
}

std::ios& takeCommand(std::istream& is, int& cmd)
{
    char firstChar;
    if (!is.get(firstChar))
    {
        return is;
    }

    is.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    switch (firstChar)
    {
        case 's': cmd = 1; break;
        case 'p': cmd = 2; break;
        case 'n': cmd = 3; break;
        case 'q': cmd = 0; break;
    }

    return is;
}