It doesn't reach the end of file when reading

Question

I have created a function to analyze a text in a file according to some rules.

void parse(const char *fileName){
    ifstream f;
    f.open("test.txt");
    char ch;
    string str = "";
    while(!f.eof()){
        f.get(ch);
        if(isspace(ch)){
            cout << ch << " -> Space." << endl;
        } else if(ch == '/'){
            cout << ch << " -> Symble." << endl;
        } else if(isalpha(ch)){
            str = ch;
            f.get(ch);
            while(isalnum(ch)){
                str += ch;
                f.get(ch);
            }
            f.putback(ch);
            cout << str << " -> String." << endl;
        } else if(isdigit(ch)){
            str = ch;
            f.get(ch);
            while(isdigit(ch)){
                str += ch;
                f.get(ch);
            }
            f.putback(ch);
            cout << str << " -> Number." << endl;
        } else {
            cout << ch << " -> Wrong value." << endl;
        }
    }
    f.close();
}

// The implementation
int main(){
    parse("test.txt");
    return 0;
}

The content of test.txt for example:

Lion King 200 26/12/1910

After searching for the problem I found when reaching the last character it is repeated continuously and does not reach the end of the file.

Debugging window:

The result:

How to make the function to reach the end of file?

---

(+) A way to solve the problem but not the best.

I have solved the problem but using a bad way in my opinion, I've put a condition in several places to know if I have reached the end of file. if(f.eof()) break;

void parse(const char *fileName){
    ifstream f;
    f.open("test.txt");
    char ch;
    string str = "";
    while(!f.eof()){
        f.get(ch);
        if(isspace(ch)){
            if(f.eof()) break;
            cout << ch << " -> Space." << endl;
        } else if(ch == '/'){
            if(f.eof()) break;
            cout << ch << " -> Symble." << endl;
        } else if(isalpha(ch)){
            str = ch;
            f.get(ch);
            while(isalnum(ch)){
                str += ch;
                f.get(ch);
                if(f.eof()) break;
            }
            cout << str << " -> String." << endl;
            if(f.eof()) break;
            f.putback(ch);
        } else if(isdigit(ch)){
            str = ch;
            f.get(ch);
            while(isdigit(ch)){
                str += ch;
                f.get(ch);
                if(f.eof()) break;
            }
            cout << str << " -> Number." << endl;
            if(f.eof()) break;
            f.putback(ch);
        } else {
            if(f.eof()) break;
            cout << ch << " -> Wrong value." << endl;
        }
    }
    f.close();
}

Is there another good way instead that?

Answer 1

When it reaches EOF (or any kind of failure), f.get(ch) does not change ch.

        while(isdigit(ch)){
            str += ch;
            f.get(ch);
        }

So you're simply stuck at this loop and keeps on appending the last available ch to str.

Make sure you're actually checking EOF instead of relying on isdigit(ch):

        while(isdigit(ch)){
            str += ch;
            if (!f.get(ch)) break;
        }

From CppReference:

basic_istream& get( char_type& ch );

Reads one character and stores it to ch if available. Otherwise, leaves ch unmodified and sets failbit and eofbit. Note that this function is not overloaded on the types signed char and unsigned char, unlike the formatted character input operator>>.

Answer 2

The general approach to dealing with reading input from a stream needs to be rethought. Any time you read from a stream, you have to verify that the read was successful before you proceed to use the value that was read from the stream.

It's better to use if (f) / while (f) instead of if (f.eof()) / while (f.eof()).

Here's an updated version of your function that should work.

void parse(const char *fileName){
   ifstream f;
   f.open("test.txt");
   char ch;
   string str = "";

   // If a char can't be read, don't enter the loop.
   while(f.get(ch))
   {
      if(isspace(ch)){
         cout << ch << " -> Space." << endl;
      } else if(ch == '/'){
         cout << ch << " -> Symble." << endl;
      } else if(isalpha(ch)){
         str = ch;

         // Read the character and proceed to isalnum only
         // if read was successful.
         while(f.get(ch) && isalnum(ch)){
            str += ch;
         }
         cout << str << " -> String." << endl;

         // Don't call putback unless the stream is still in a no error state.
         if ( f ) {
            f.putback(ch);
         }
      } else if(isdigit(ch)){
         str = ch;
         // Read the character and proceed to isdigit only
         // if read was successful.
         while(f.get(ch) && isdigit(ch)){
            str += ch;
         }
         cout << str << " -> Number." << endl;

         // Don't call putback unless the stream is still in a no error state.
         if ( f ) {
            f.putback(ch);
         }
      } else {
         if(!f) break;
         cout << ch << " -> Wrong value." << endl;
      }
   }
   f.close();
}

---

A few suggestions to make the function simpler.

Suggestion 1

Combine

ifstream f;
f.open("test.txt");

to one line

ifstream f("test.txt");

Suggestion 2

You are passing fileName as argument but have hard coded file name test.txt in the function. I think it will be better to use fileName.

ifstream f(fileName);

Suggestion 3

There is no harm in the line

f.close();

but it is unnecessary. The file will be closed when the function returns.

Suggestion 4

You can process the contents of the file by reading lines of text and processing the lines instead of having function calls to read one character at a time in multiple places. It will also allow you to make the function simpler. You can create smaller functions to make the code simpler.

std::string::iterator parseAlphaNumeric(std::string::iterator start,
                                        std::string::iterator end)
{
   // Iterate over the string until we reach the end of the
   // line or the first character that is not an alphanumeric character.
   std::string::iterator iter = start;
   for ( ++iter; iter != end && isalnum(*iter); ++iter );

   // Create a string using the itertors instead of
   // adding to the string one character at a time.
   std::cout << std::string(start, iter) << " -> String." << std::endl;

   // The iterator now points to either the end of the line
   // or the first character that is not an alphanumeric character.
   return iter;
}

std::string::iterator parseNumeric(std::string::iterator start,
                                   std::string::iterator end)
{
   // See comments in the previous function.

   std::string::iterator iter = start;
   for ( ++iter; iter != end && isdigit(*iter); ++iter );
   std::cout << std::string(start, iter) << " -> Number." << std::endl;
   return iter;
}

void parseLine(std::string::iterator iter,
               std::string::iterator end)
{
   while ( iter != end )
   {
      char ch = *iter;
      if(isspace(ch)){
         std::cout << ch << " -> Space." << std::endl;
         ++iter;
      } else if(ch == '/'){
         std::cout << ch << " -> Symble." << std::endl;
         ++iter;
      } else if(isalpha(ch)){
         iter = parseAlphaNumeric(iter, end);
      } else if(isdigit(ch)){
         iter = parseNumeric(iter, end);
      } else {
         std::cout << ch << " -> Wrong value." << std::endl;
         ++iter;
      }
   }
}

void parse(const char *fileName){
   std::ifstream f(fileName);

   // Read lines of text and process each line.
   std::string line;
   while(getline(f, line))
   {
      parseLine(line.begin(), line.end());
   }
}

Answer 3

Most likely stream never ends since you put symbols back:

f.get(ch);
// ...
f.putback(ch);

Also note that you should check stream status after every get call, not just once per loop iteration.