Return to Answer

• 1

• 3k
• 1
• 22
• 25

double Student::getSize()
{
 return sizeof(firstName) + sizeof(lastName) + sizeof(gpa) + sizeof(id);
}

This is wrong for several reasons:

• sizeof evaluated to a std::size_t, an unsigned integer type. Not a floating point.
• Adding the sizes of a structure's members is generally not sufficient to get the size of the structure. You need to consider padding.
• The size of a structure doesn't include the this of things pointed to by its members, directly or indirectly. Here, sizeof on the string members gives you the size of your standard library's implementation of the std::string structure, which is of very little use: in particular it is unrelated to the size of the actual string contents. It is a compile-time constant.

Just remove that function altogether. If you need the size of the struct, use sizeof(Student).

Student::Student(std::string studentInformation)
{
 stringstream studentStream(studentInformation); // a stream of student information
 studentStream >> firstName;
 studentStream >> lastName;
 studentStream >> id;
 studentStream >> gpa;
}

Each and every line in that function can fail. You need to do more error checking.

std::vector<Student> exposeStudentVector() const;

This is not a good name. Just call it students() and use another name for the member. (Some people like m_ prefixes, some a trailing _ for example.)

ifstream studentFile(filePath);

This can fail. You must check if it worked or not and find a way to convey that information to the caller - since you're in a constructor your only two options are an exception and an out parameter (and this second one isn't very appetizing since you're left with an object in a strange state).

while (!studentFile.eof())

This is almost always wrong. See: Why is iostream::eof inside a loop condition considered wrong?.

studentFile.close();

The destructor of studentFile will take care of that.

datafile.write((char*)&s, sizeof(s));
datafile.read((char*)s, sizeof(*s));

These don't work at all. While this can work for plain data containers (C structs essentially, but even then this is not portable - see alignment and padding), they will not do the right thing for anything even slightly complicated (e.g. pointers). std::string contains pointers inside it, you're saving raw pointer values into an binary file, and then reading those back in. Those pointer values have no reason to be valid in another process. Even in the same process, if the pointers have been freed (objects pointed-to deleted for instance), you're just pointing at random memory. On top of that, the constructors for the std::string members aren't run.

And you're again failing to check that the operations succeeded for the read and write calls. (Plus an arbitrary limitation to exactly 13 entries.)

If you want to serialize structures, you should look into serialization libraries, it is in fact a non-trivial task to get right. Google Protobuf and boost::serialization come to mind. If you want to do it yourself, you'll need to really think about how to save and restore every member in a safe way. An example for a string is to write out its length (in binary), then the contents. Another approach is just writing out the zero-terminated string contents, but that's a bit more of a pain to deserialize and validate.

Make sure you test each part of your code independently, in new processes (i.e. doing a write/read in the same process will hide all kinds of pointer bugs that are actually undefined behavior and could crash in fancy fashions - or not unfortunately). Also test invalid inputs. Also test things like input file not being there at all, wrong permissions that prevent the output files from being generated, etc. Your program should behave correctly, or at least sanely, in these circumstances. Entering an infinite loop is not nice.

Make sure you check the return values of all functions that can fail. For starters: all I/O operations can fail, all memory allocations can fail, all parsing operations can fail, all system calls can fail.