2

I noticed in my C++ code that anytime I close an std::ofstream object I'm unable to reopen the file I closed with std::ifstream. std::ifstream's open function will always fail.

Is there anything 'extra' I can do to ensure that my std::ofstream object closes properly?

Someone's probably going to ask to see my specific code so for the sake of keeping this post small I've pastied it here. In my code after running through case a or d all std::ifstream open calls fail. (Prior to posting this question I had several people play with my code who were unable to conclude anything other than that std::ofstream close failing for unknown reasons)

Thanks in advance to any and all responses received.

Code is

#include <iostream>
#include <fstream>
#include <string>

using namespace std;

typedef struct Entry
{
   string Name;
   string Address;
   string Phone;   
};

int main()
{
   bool exit = false, found = false;
   char input, ch;
   string stringinput, stringoutput;
   ifstream fin, ibuffer;
   ofstream fout, obuffer;
   Entry buffer;

   while(!exit)
   {
      cout << "Welcome to the Address Book Application!" << endl << endl;
      cout << "\tSelect an option:" << endl << endl;
      cout << "\tA -- Add New Entry" << endl;
      cout << "\tD -- Delete an Entry" << endl;
      cout << "\tS -- Search for an Existing Entry" << endl;
      cout << "\tE -- Exit" << endl << endl;

      cin >> input;

      switch(input)
      {
         case 'a':
         case 'A':
         cin.ignore(255,'\n');//Apparently necessary because an extra new line carrys over in the cin stream
         system("cls");
         //Get Information from User
         cout << "Enter Phone Number: ";
         getline(cin, buffer.Phone);
         cout << endl << "Enter Name: ";
         getline(cin, buffer.Name);
         cout << endl << "Enter Address: ";
         getline(cin, buffer.Address);
         /*Copy existing database into a buffer. In other words, back it up*/
         fin.open("address.txt");
         if(!fin)
         {
            fin.close();
            fout.open("address.txt");
            fout << buffer.Phone << endl << buffer.Name << endl << buffer.Address << endl;
         }
         if(fin)
         {
            obuffer.open("buffer.txt");
            while(fin && fin.get(ch))
               obuffer.put(ch);
            fin.close();
            obuffer.close();
            /*Copy buffer to new database file*/
            ibuffer.open("buffer.txt");
            fout.open("address.txt");//This removes all of the previously existing info from database.txt
            while(ibuffer && ibuffer.get(ch))
               fout.put(ch);
            ibuffer.close();
            remove("buffer.txt");//Delete the buffer
            fout << endl << buffer.Phone << endl << buffer.Name << endl << buffer.Address << endl;
         }

         buffer.Phone.erase();
         buffer.Name.erase();
         buffer.Address.erase();
         fout.close();
         break;

         case 'd':
         case 'D':
         cin.ignore(255,'\n');//Apparently necessary because an extra new line carrys over in the cin stream
         system("cls");
         cout << "Enter the phone number of the entry to delete: ";
         cin >> stringinput;
         fin.open("address.txt");
         if(!fin)
         {
            cout << endl << "No entries exist!";
            fin.close();
            break;
         }
         obuffer.open("buffer.txt");
         /* Copy everything over into the buffer besides the account we wish to delete */
         while(!fin.eof())
         {

            fin.read(&ch, sizeof(char));

            if(ch != '\n' && ch != '\0')
            stringoutput += ch;

            if(ch == '\n' || ch == '\0')
            {
               if(stringinput.compare(stringoutput))
               {
                  stringoutput += ch;
                  obuffer << stringoutput;
                  stringoutput.erase();
               }

               if(!stringinput.compare(stringoutput))
               {
                  stringoutput += ch;
                  getline(fin, stringoutput);
                  getline(fin, stringoutput);
                  fin.read(&ch, sizeof(char));//Get rid of the extra '\n'
                  stringoutput.erase();
               }

            }

         }

         //Hack: Copy the last line over since the loop for some reason doesn't
         obuffer << stringoutput;
         stringoutput.erase();

         fin.close();
         obuffer.close();

         fout.open("address.txt");
         ibuffer.open("buffer.txt");

         while(ibuffer && ibuffer.get(ch))
            fout.put(ch);

         ibuffer.close();
         fout.close();
         remove("buffer.txt");

         cout << endl << "Entry " << stringinput << " deleted successfully!" << endl;
         stringinput.erase();
         stringoutput.erase();
         break;

         case 's':
         case 'S':
         cin.ignore(255,'\n');//Apparently necessary because an extra new line carrys over in the cin stream
         system("cls");

         found = false;

         fin.open("address.txt");
         if(!fin)
         {
            cout << "No entries currently exist!" << endl << endl;
            fin.close();
            break;
         }

         cout << "Enter the phone number to search for: ";
         cin >> stringinput;

         while(!fin.eof())
         {
            fin.read(&ch, sizeof(char));

            if(ch != '\n' && ch != '\0')
               stringoutput += ch;

            if(ch == '\n' || ch == '\0')
            {
               if(!stringinput.compare(stringoutput))
               {
                  found = true;
                  break;
               }

               stringoutput.erase();
            }

         }

         if(found)
         {
            cout << "Phone Number: " << stringinput << endl;
            getline(fin, stringoutput);
            cout << "Name: " << stringoutput << endl;
            getline(fin, stringoutput);
            cout << "Address: " << stringoutput << endl << endl;
         }

         if(!found)
         {
            stringoutput.erase();
            cout << endl << stringinput << " is not in the address book!" << endl << endl;
         }

         stringinput.erase();
         stringoutput.erase();
         fin.close();
         break;

         case 'e':
         case 'E':
         exit = true;
         break;

         default:
         system("cls");
         cout << input << " is not a valid option." << endl << endl;
         break;
      }

   }

   return 0;

}
Steve Townsend
  • 53,498
  • 9
  • 91
  • 140
Rob S.
  • 3,599
  • 6
  • 30
  • 39
  • It would help to know which stream objects are the cause of your grief and at which line in the code you hit the error. – Marcelo Cantos Oct 30 '10 at 13:42
  • In my code specifically after I fout.close() I'm unable to fin.open() – Rob S. Oct 30 '10 at 13:44
  • 4
    We still like to keep the code in the question so the SO post is more self-contained and useful when that paste inevitably dies. Please whittle down to a smaller testcase and include in the question. –  Oct 30 '10 at 13:45
  • 1
    I should offer the point that declaring all your variables at the top is not idiomatic C++, and may be to blame for a) the bug itself, and b) the difficulty you and your colleagues are having in diagnosing the bug. In C++, it is very important to minimise the scope of stack objects. Also, where possible, initialise as part of construction instead of default initialisation followed by `open()`. – Marcelo Cantos Oct 30 '10 at 13:45
  • 2
    Read about [RAII](http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization). Read carefully, make sure you understand, and then modify your code accordingly. Then see if the error is still present. – Björn Pollex Oct 30 '10 at 13:46
  • There are two points in the code where you call `fout.close()` and both are followed by `fin.open()`. – Marcelo Cantos Oct 30 '10 at 13:48
  • Another tip: Don't copy files with tons of `get()` and `put()` calls. Use the `read()` and `write()` functions for chunkier I/O. Also, since you do copying all over the place, refactor it into a function. – Marcelo Cantos Oct 30 '10 at 13:51
  • I'm with Roger, Marcelo, and Space_C0wb0y. Pay attention to the stylistic hints you got, and boil the problem down to 20 lines that reproduce the error. Then come back with that. – sbi Oct 30 '10 at 13:51
  • @Roger Pate so far I have not been able to reproduce this in several small attempts I've made. Marcelo Cantos, excellent suggestion I'll get to work on that. Space_C0wb0y thanks, if the previous suggestions don't help I'll try that. – Rob S. Oct 30 '10 at 13:52
  • 2
    You might consider refactoring your ~200 line main method into smaller functions. I often find doing that makes behavior like you've described easier to pinpoint. – Sam Miller Oct 30 '10 at 13:53
  • @Rob: I'd rather see 300 lines in the question than a paste link. Of course, I'd rather see 50, but when you can't manage that... ;) –  Oct 30 '10 at 13:57
  • I don't think your code is too large for this site, I pasted it in up above – Steve Townsend Oct 30 '10 at 14:21
  • 1
    Thanks guys, using your suggestions I've managed to get it working. Working code is here: http://pastie.org/private/3fs09qqj8toes3rzsq I plan to continue to implement the remaining suggestions to make my code as efficient and clean as possible. Thanks a ton! – Rob S. Oct 30 '10 at 14:26

1 Answers1

4

The best way to ensure your streams are reset at each point in this processing is not to explicitly close them, but to declare them at the point of use and allow them to go out of scope. This is the RAII point that was made in comments. In your case this means moving the declaraion from the top of the function to inside each arm of the switch where they are used. This way you can be sure that each file is cleanly closed when you exit from a particular case, as the ofstream and ifstream close the file during destruction on scope exit.

Another thing I noticed is that you are testing a strange thing after file open:

 fin.open("address.txt");
 if(!fin)

This tests for a bad stream but I don't think it's complete - if you are testing for a successful open, you should also test

 fin.open("address.txt");
 if (!fin.is_open())

All of your file processing loops and open() calls need to check for fin.fail() or fout.fail() as well as fin.eof(), to rule out file access error as an unhandled condition that is confusing your observed behaviour.

At this point, I suggest you fix these obvious misunderstandings, and come back with more specific questions if you cannot get it working.

Steve Townsend
  • 53,498
  • 9
  • 91
  • 140