The generally correct answer is use a std::vector as it assists in managing ownership and the lifetime of storage as well as tracking the capacity and resizing itself as needed. But let's take a walk through fillPlayers to explain what is going wrong.
Player * fillPlayers()
{
ifstream file1;
Player * fullPlayerPointer = new Player[244];
Dynamically allocated 244 Players. Dynamic storage has manually controlled lifetime and will remain valid until such time as it is freed. Unfortunately this allocated storage is never used and not put away correctly later with delete[] This allocation will be "leaked". As dire as this sounds, this is the "right" way to allocate the storage, but as we will see, the function uses it incorrectly.
Player ourPlayers[244];
Statically allocated 244 Players as a temporary variable. This variable will only exist inside the innermost set of surrounding curly braces {}. Afterward it will be rendered invalid. This means that references to ourPlayers should not be returned from this function as ourPlayers will be rendered invalid at the return from the function and before the caller can make use of it.
file1.open("Players.txt");
if (file1.fail())
{
cerr << "file1 did not open";
}
Always test before using, so the above is almost right. It is rendered redundant by the next line performing a nearly identical test. This could be a good place to put an else, but the code is more easily read with if (file1.is_open()) followed by an else to print the error message if it is not open.
Why do I say this? Because the programmer's intent with is_open is much easier to discern than with the broader term fail.
if (file1.is_open())
{
while (!file1.eof())
Read Why is iostream::eof inside a loop condition considered wrong? for details on why this is almost always the wrong solution to looping through a file.
{
string size;
getline(file1, size);
Always test a read to ensure it succeeded. In addition, the file size was read and then not converted into an integer to use in the loop that follows.
for (int i = 0; i < 244; i++)
Regardless of how many entries there are in this file 244 will always be read. If the file does not have at least 244 entries, the read will fail and as the reads are not being checked for success, garbage will be stored in the array.
Also note that there is a loop iterating through 244 entries in the file that is surrounded by a loop that will attempt to read again until the EOF flag is set. Most likely you only want one such loop.
{
file1 >> ourPlayers[i].playerID;
file1 >> ourPlayers[i].name;
}
}
file1.close();
}
fullPlayerPointer = &ourPlayers[0];
The pointer to the dynamic allocation made earlier is overwritten by a pointer to the temporary allocation ourPlayers. A reference to long-term storage has been replaced by a reference to storage that is about to go out of scope and become invalid.
It is possible that OP intended this to copy the data in the short term storage to the long term storage, but unfortunately that's not what the compiler was told to do, and it's not really worth doing. It would be much more useful to directly read the file into the long term storage.
return fullPlayerPointer;
Returns from the function and gives an invalid array to the caller.
}
Lot to fix in there.
Here's a very simple approach that fixes all of the above problems but exposes a few more:
Player * fillPlayers()
{
ifstream file1("Players.txt");
Player * players = nullptr;
if (file1.is_open())
{
int size;
if (file1 >> size)
{
players = new Player[size];
int index = 0;
while (file1 >> players[index].playerID >> players[index].name
&& index < size)
{
}
// extra brains needed here to handle premature read failure.
}
file1.close();
}
else
{
cerr << "file1 did not open";
}
return players; // only a pointer to the array was returned. How big the
// array is and how many items are actually in it is lost
}
This is where std::vector really becomes awesome. It knows how big it is and how full it is. An array doesn't.
Now, assuming std::vector is not allowed, and Paul McKenzie has already covered what to do if it isn't, the smart thing to do is make a very simple wrapper around the array to get some modicum of the safety and ease of use vector provides.
class stupidvector
{
Player *players;
size_t capacity; // a typical vector implementation uses a pointer for
// this to make iteration easier
size_t size; // vector uses pointer here, too.
public:
stupidvector();
stupidvector(size_t size);
// correctly copy a stupid vector Rule of Three. In this case, don't
// allow it to be copied.
stupidvector(const stupidvector& src)=delete;
// correctly move a stupid vector Rule of Five
stupidvector(stupidvector && src);
// release storage
~stupidvector();
// add an item to the end
void push(const Player & player);
// how big is it?
size_t getcapacity();
// how full is it?
size_t getsize();
// make it act like an array
Player & operator[](size_t index);
// correctly assign. Rule of Three. again we don't want to copy these,
// but if we did, look at Copy and Swap Idiom for a neat solution
stupidvector& operator=(const stupidvector& src) = delete;
// correctly move
stupidvector& operator=(stupidvector && src);
};
Pay special attention to the Rules of Three and Five
