C++ Program crashes when reading object from random access file

I have the following User.h that holds several attributes (strings). User.cpp has all the definitions.

//User.h
#ifndef USER_H
#define USER_H
#include<iostream>
#include <cstring>

using namespace std;

class User{

  开发者_StackOverflowstring username;

  public:
         User();
         string getUsername() const;                      
         void setUsername(string);

};
#endif

I'm using a another class, "File" to insert new users/view users from a randomly accessed .dat file

//File.h
#ifndef FILE_H
#define FILE_H
#include "User.h"

class File{
    public:

             void loadUser(); 
             bool addUser(User&); 

};
#endif

File class definitions

//File.cpp
#include<cstring>
#include<iostream>
#include<iomanip>
#include<fstream>

#include "User.h"
#include "File.h"

using namespace std;

User tempUser;
fstream usersFile;

void File::loadUser(){
     usersFile.open("users.dat", ios::in | ios::binary);

     usersFile.seekg(0);  

     // commenting the following lines prevented the issue
     usersFile.read(reinterpret_cast<char *>(&tempUser), sizeof(tempUser)); 

     cout<<tempUser.getUsername().c_str(); 

     usersFile.close();
}

bool File::addUser(User& user){

     usersFile.open("users.dat", ios::out | ios::ate | ios::binary);

     // no issue when writing to file
     usersFile.write( reinterpret_cast<const char *>(&user), sizeof(user));

     usersFile.close();

     cout<<"User added";  
}

I'm getting the above mentioned issue when running. No compilation issue though.

Is there any issue when dealing with objects that has "string attributes" inside, in handling?

Please help

I think the issue is that you are mixing C++ code with a C mindset. What you should really be doing here is use the extraction operator, opeartor>>(), along with the C++ IO streams. This, as opposed to using the C standard IO along with the read() function. For writing an object, use the insertion operator operator<<() instead of the C write() function.

For working with strings use std::string. This class provides operator<<() and operator>>(). So, you can say std::string s and then io << s and io >> s where io is some C++ IO stream object. This will do The Right Thing(tm). The philosophy here is that the std::string class knows better than you, a user, how to serialize a std::string object. So let it do it, with the << and >> operators.

Going on with the idea, you, as the author of User, know better than anyone else how to serialize a User object. So provide the << and >> operators for users of your class, as a service. "Users of your class" might well be you one week from now, when you have completely forgot how to properly serialize a User object. (Or, you think you remember but in practice you forgot a detail, causing a bug in your code). Example:

// in User.h
#include <string>
#include <iosfwd>  // forward declarations of standard IO streams

namespace mine {
class User {
    User(const std::string& name) : username(name) { }
    friend std::ostream& operator<<(std::ostream&, const User&);
    friend std::istream& operator>>(std::istream&, User&);

private:
    std::string username;
};

std::ostream& operator<<(std::ostream& out, const User& u)
{
    return out << u.username;
}

std::istream& operator>>(std::istream& in, User& u)
{
    return in >> u.username;
}
}  // namespace mine

From here on, to save a user to a file you say

std::ofstream f("filename");
User u("John");
f << u;

That's it. To read a user:

std::ifstream f2("filename");
f2 >> u;

It's a good practice to wrap your code in a namespace. IDEs give a good visualization of this issue, by showing how many symbols are visible with the auto-complete feature. You get to see how much of a mess there is in the global scope. By wrapping your code in a namespace you group it under a scope name, saving some more mess in the global name scope. It's just about being tidy. If you put your code in your own namespace then you can choose any name you want for a function, a class or a variable, so long as you haven't chosen it before. If you don't put it in a namespace then you need to share names with others. It's like a skunk declaring his own territory, only without the bed smell.

On that note I suggest you take off that using namespace std from your header. This brings all the symbols in the std namespace into scope of all files that #include the header. It's a bad practice. Only say using namespace std in implementation files, if you wish, but not in header files.

Granted, some will say even this is a bad idea. I personally think it's fine if you're aware of the fact you might have name clashes in that particular implementation file. But at least you know where that using statement is: it's in your implementation file, and it only causes clashes in that implementation file. It's sort of a gun, (a plastic water gun, but a gun nonetheless), only you can only shoot (wet) your own feet and no one else's. Which in my opinion is perfectly fine.

You can't read non-POD types like that. A string is not a POD type. What are POD types in C++?

There are a few ways to read in strings properly, depending upon how they were stored.

For text files:

If the string is just a single word, separated on both sides by whitespace, you can use the plain old >> operator. If it's more than one word, you can store it on it's own line, and use getline.

For binary files:

Store the string in null terminated form. Read it one character at a time, checking for the null character. Or, prepend the string with an integer storing it's size. When you read it, first read in the integer, then read in that many characters.

string is an object, which means you're not writing it's contents.

Try writing an User and examine the file to see what I mean. What you're then reading are some pointers, that point to invalid memory locations.

Yes, the std::string class is not plain old data, in other words it contains pointers. If you save/load the string class in this manner, the data pointed to won't be loaded/saved, only the value of the pointer.

Additionally sizeof(tempUser) won't include the size of the text pointed to by the strings.

Your solution is to change how you're reading/writing data. One way is to use boost::serialization which handles datatypes like std::string. Another way would be to write each string to a separate line in the text document yourself (not in binary mode) and then use readline to read them back in.

Rather than construct your own custom serialization code, something like Google Protocol Buffers might do what you want with less effort. It's ideal for passing around simple structured data from place to place.