Reading from file and outputting to console through vector

Question

I'm trying to read from this file file.txt which contains contents of the contestants who participated in the long jump event during the Olympic games.

The file is in the format of [First Name] [Last Name] [Nationality] [Distance]

There are 40 contestants in this file. I'm trying to organize them such that there is a vector of pointers of athletes, 40 to be precise. Then dynamically allocate them to the heap. Each athlete is one object in the vector.

Once each athlete object is entered into the vector, I wish to output all the contents of the vector onto the console through a for loop.

However, as it currently stands, in my code I do have 40 objects allocated to the vector, but its the same one being repeated 40 times. The last object is also being repeated twice for some reason.

Any help would be greatly appreciated! Thanks in advance.

Test.cpp

#include <iostream>
#include<fstream>
#include<vector>
#include<string>
#include "Person.h"
#include "Athlete.h"
using namespace std;

vector<Athlete*>  athletesList(40);

//overload the operator << to be used for printing the record Objects
ostream& operator<<(ostream& out, Athlete& a) {
    out << a.getFirstName() << " " << a.getLastName() << " " << a.getNationality() << " " << a.getDistance() << "\n";
    return out;
}

void readAthletesFromFile() {

    fstream athlethesFile;
    athlethesFile.open("file.txt");

    string tmpFirstName;
    string tmpLastName;
    string tmpNationality;
    string tmpDoubleDistance;

    while (!athlethesFile.eof()) {

        athlethesFile >> tmpFirstName;
        athlethesFile >> tmpLastName;
        athlethesFile >> tmpNationality;
        athlethesFile >> tmpDoubleDistance;

        double tmpDistance = stod(tmpDoubleDistance);

        for (int i = 0; i < 40; i++) {
            athletesList[i] = new Athlete(tmpFirstName, tmpLastName, tmpNationality, tmpDistance);
        }

        cout << *athletesList[0];



    }

}
int main()
{
    readAthletesFromFile();
    system("pause");

}

File.txt

Aleksandr Menkov Russia 8.09
Aleksandr Petrov Russia 7.89
Alyn Camara Germany 7.72
Arsen Sargsyan Armenia 7.62
Boleslav Skhirtladze Georgia 7.26
Christian Reif Germany 7.92
Christopher Tomlinson Great_Britain 8.06
Damar Forbes Jamaica 7.79
Eusebio Caceres Spain 7.92
George Kitchens United_States 6.84
Godfrey Khotso-Mokoena South_Africa 8.02
Greg Rutherford Great_Britain 8.08
Henry Frayne Australia 7.95
Ignisious Gaisah Ghana 7.79
Li Jinzhe China 7.77
Lin Ching-Hsuan-Taipei China 7.38
Louis Tsatoumas Greece 7.53
Luis Rivera Mexico 7.42
Marcos Chuva Portugal 7.55
Marquise Goodwin United_States 8.11
Mauro-Vinicius da-Silva Brazil 8.11
Michel Torneus Sweden 8.03
Mitchell Watt Australia 7.99
Mohamed Fathalla-Difallah Egypt 7.08
Mohammad Arzandeh Iran 7.84
Ndiss Kaba-Badji Senegal 7.66
Povilas Mykolaitis Lithuania 7.61
Raymond Higgs Bahamas 7.76
Roman Novotny Czech-Republic 6.96
Salim Sdiri France 7.71
Sebastian Bayer Germany 7.92
Sergey Morgunov Russia 7.87
Stanley Gbagbeke Nigeria 7.59
Stepan Wagner Czech-Republic 7.5
Supanara Sukhasvasti Thailand 7.38
Tyrone Smith Bermuda 7.97
Vardan Pahlevanyan Armenia 6.55
Viktor Kuznyetsov Ukraine 7.5
Will Claye United_States 7.99
Zhang Xiaoyi China 7.25

Expected Output

Ex: *athletesList[0] = Aleksandr Menkov Russia 8.09
    *athletesList[10]= Godfrey Khotso-Mokoena South_Africa 8.02

Current Output

Ex: *athletesList[0] = 
Aleksandr Menkov Russia 8.09
Aleksandr Petrov Russia 7.89
Alyn Camara Germany 7.72
Arsen Sargsyan Armenia 7.62
Boleslav Skhirtladze Georgia 7.26
Christian Reif Germany 7.92
Christopher Tomlinson Great_Britain 8.06
Damar Forbes Jamaica 7.79
Eusebio Caceres Spain 7.92
George Kitchens United_States 6.84
Godfrey Khotso-Mokoena South_Africa 8.02
Greg Rutherford Great_Britain 8.08
Henry Frayne Australia 7.95
Ignisious Gaisah Ghana 7.79
Li Jinzhe China 7.77
Lin Ching-Hsuan-Taipei China 7.38
Louis Tsatoumas Greece 7.53
Luis Rivera Mexico 7.42
Marcos Chuva Portugal 7.55
Marquise Goodwin United_States 8.11
Mauro-Vinicius da-Silva Brazil 8.11
Michel Torneus Sweden 8.03
Mitchell Watt Australia 7.99
Mohamed Fathalla-Difallah Egypt 7.08
Mohammad Arzandeh Iran 7.84
Ndiss Kaba-Badji Senegal 7.66
Povilas Mykolaitis Lithuania 7.61
Raymond Higgs Bahamas 7.76
Roman Novotny Czech-Republic 6.96
Salim Sdiri France 7.71
Sebastian Bayer Germany 7.92
Sergey Morgunov Russia 7.87
Stanley Gbagbeke Nigeria 7.59
Stepan Wagner Czech-Republic 7.5
Supanara Sukhasvasti Thailand 7.38
Tyrone Smith Bermuda 7.97
Vardan Pahlevanyan Armenia 6.55
Viktor Kuznyetsov Ukraine 7.5
Will Claye United_States 7.99
Zhang Xiaoyi China 7.25
Zhang Xiaoyi China 7.25

You should read [Why is iostream::eof inside a loop condition (i.e. `while (!stream.eof())`) considered wrong?](https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-i-e-while-stream-eof-cons) to fix your second problem (_"The last object is also being repeated twice for some reason"_) — Thomas Sablik, Jan 21 '20 at 14:54
Why do you use `vector athletesList(40);` instead of `vector athletesList(40);`. Where do you clean up the dynamic allocated memory? Why do you print 40 times the same vector element with `cout << *athletesList[0];`? — Thomas Sablik, Jan 21 '20 at 14:58
change the while from while(!athlethesFile.eof()) to while(!athlethesFile) remove the for loop and use atheletesList.push_back(new Athlete(....)). That's what i can think of. Hope it helps. — octopus, Jan 21 '20 at 15:15
@octopus That's actually better, but if reading fails after inputting e. g. last name, you'd produce an incomplete athlete that way... — Aconcagua, Jan 21 '20 at 15:17
@octopus changing `while(!athlethesFile.eof())` to `while(!athlethesFile)` won't change the problem. In both cases you would read and store one invalid entry. — Thomas Sablik, Jan 21 '20 at 15:22
@ThomasSablik I wrote vector athletesList(40) Because I wanted to create 40 pointers in the vector that would point to each individual object. This way they would be allocated in the heap. I still intend to do that, in this solution are the objects being stored in the heap or in the stack? I believe its in the stack because I don't see the 'new' keyword here. — bob, Jan 21 '20 at 15:58
A vector stores its elements on the heap. AFAIK you can't dynamically allocate memory on the stack. You don't need to allocate memory and store pointers. The vector creates the elements on the heap, manages the memory and cleans up at the end. You should read [R.11: Avoid calling new and delete explicitly](http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-newdelete) — Thomas Sablik, Jan 21 '20 at 16:00
@ThomasSablik I was also wondering why I can't access for example the 10th element in the vector when I do something like athletesList.at(9) — bob, Jan 21 '20 at 16:10
@ThomasSablik Oh yes it does. I had my cout statement in the while loop. One final question, I have for you. Does this snippet of code use dynamic memory allocation and then de-allocate them after its use? Correct me if I'm wrong but I don't think so? — bob, Jan 21 '20 at 16:17
My code allocates dynamic memory using the vector and deallocates the memory after the vector goes out of scope. Your code allocates dynamic memory for 40 pointers using the vector. In the loop it allocates 1600 elements but it doesn't deallocate memory for these elements. That's called memory leak. You should check your code with valgrind to see the problem. — Thomas Sablik, Jan 21 '20 at 16:28
@ThomasSablik Oh ok. so to deallocate the memory, I would have to use the 'delete' keyword and go through a for loop. My intention wasn't to create 1600 elements but only 40. So how would my code need to change to accommodate only the 40 elements instead of the 1600? — bob, Jan 21 '20 at 16:31
It's all described and solved in my answer. If you are not sure that the vector handles dynamic memory you can read [std::vector](https://en.cppreference.com/w/cpp/container/vector). You can access the memory with [std::vector::data](https://en.cppreference.com/w/cpp/container/vector/data) — Thomas Sablik, Jan 21 '20 at 16:39

Thomas Sablik · Accepted Answer · 2020-01-21T16:15:49.447

Avoid calling new and delete explicitly to dynamically allocate memory. You don't need it in combination with STL containers. A container allocates and manages the memory on the heap.
The for loop is not necessary. For each element you read you overwrite all elements with the last read element.
You print the first element from the vector for each element you read:

Change
```
cout << athletesList[0];
```
to
```
cout << athletesList.back();
```
You can't
```
while (!athlethesFile.eof()) {
```
because eof is set after you tried to read and it failed. You have to check if the read was successful after the read. Why is iostream::eof inside a loop condition (i.e. while (!stream.eof())) considered wrong?
Don't include Person.h if you don't use it.
You should avoid system(). Usually it's not portable.

In your current code you read 40 elements in a loop. In each loop iteration you allocate 40 elements for that vector. That means that you allocate memory for 1600 elements. You do not clean that up. That is a serious memory leak.

#include "Athlete.h"
#include <fstream>
#include <iostream>
#include <string>
#include <vector>
using std::cerr;
using std::cout;
using std::fstream;
using std::ostream;
using std::string;
using std::vector;

//overload the operator << to be used for printing the record Objects
ostream& operator<<(ostream& out, Athlete& a) {
    out << a.getFirstName() << " " << a.getLastName() << " " << a.getNationality() << " " << a.getDistance() << "\n";
    return out;
}

auto readAthletesFromFile() {
    vector<Athlete> athletesList;
    athletesList.reserve(40);

    fstream athlethesFile("file.txt");

    if (!athlethesFile) {
        cerr << "Could not open file\n";
        return athletesList;
    }

    string tmpFirstName;
    string tmpLastName;
    string tmpNationality;
    string tmpDoubleDistance;

    while (true) {
        athlethesFile >> tmpFirstName;
        athlethesFile >> tmpLastName;
        athlethesFile >> tmpNationality;
        athlethesFile >> tmpDoubleDistance;

        if (!athlethesFile) break;

        auto tmpDistance = stod(tmpDoubleDistance);
        athletesList.emplace_back(tmpFirstName, tmpLastName, tmpNationality, tmpDistance);

        //cout << athletesList.back();
    }
    return athletesList;
}

int main() {
    auto athletesList = readAthletesFromFile();
    cout << athletesList[9];
}

The eof-check still is bad. What if some IO-error occured without setting eof? Checking `if(!file)` would catch that as well. Apart from, it's pretty close to what I would have proposed myself... — Aconcagua, Jan 21 '20 at 15:15
Hm... Maybe I'm picky, but if one uses STL containers, one *does* use dynamic memory allocation, doesn't one? Maybe it's due to my background, but if I hear that sentence, then STL is out as well (only static or local storage allowed – that's usually the case on micro controllers)... — Aconcagua, Jan 21 '20 at 15:44
@Aconcagua But you are right and a compiler is also very nitpicky. I reformulated it. — Thomas Sablik, Jan 21 '20 at 15:49

score 0 · Answer 2 · answered Jan 22 '20 at 06:07

I'm going to posit that @Thomas Sablik started out in the right direction, but then at least in my opinion, he kind of dropped the ball, so to speak. In particular, I think this part:

//overload the operator << to be used for printing the record Objects
ostream& operator<<(ostream& out, Athlete& a) {
    out << a.getFirstName() << " " << a.getLastName() << " " << a.getNationality() << " " << a.getDistance() << "\n";
    return out;
}

...is absolutely great! Even if I really wanted to say bad things about it, just about the worst I could probably come up with would be that the Athlete should be passed by const reference (and being honest, that's a pretty minor quibble).

So we have great code to write an Athlete object. But for some reason, our code for reading Athlete objects isn't the same at all. Why not?

I believe the code for reading should be almost a mirror image of the code for writing.

std::istream &operator>>(std::istream &in, Athlete &a) { 
    in >> a.firstName >> a.lastName >> a.nationality >> a.distance;
    return in;
}

Using that, we can leave almost all the memory management to the standard library. In fact, there's probably no need for the readAthletesFromFile at all. Reading in a file becomes something like this:

std::ifstream input("file.txt");
std::vector<Athlete> list{std::istream_iterator<Athlete>(input), {}};

That's it. That reads in the entire file. Well, it could stop sooner if it encountered bad input, but it'll read as much as it can anyway.

So let's consider an example: we'll read in the data and print out the top 5 distances (and the information for each of those records):

class Athlete {
public:
    friend std::ostream& operator<<(std::ostream& out, Athlete const& a) {
        out << a.firstName << " " << a.lastName << " " << a.nationality << " " << a.distance << "\n";
        return out;
    }

    friend std::istream &operator>>(std::istream &in, Athlete &a) { 
        in >> a.firstName >> a.lastName >> a.nationality >> a.distance;
        return in;
    }

    bool operator<(Athlete const &other) const { 
        return distance < other.distance;
    }

private:

    std::string firstName, lastName, nationality;
    double distance;
};

int main() {
    std::ifstream in("file.txt");
    std::vector<Athlete> list{std::istream_iterator<Athlete>(in), {}};

    std::partial_sort(list.begin(), list.end()-5, list.end());
    std::copy(list.end()-5, list.end(), std::ostream_iterator<Athlete>(std::cout, ""));
}

Result:

Mauro-Vinicius da-Silva Brazil 8.11
Marquise Goodwin United_States 8.11
Aleksandr Menkov Russia 8.09
Greg Rutherford Great_Britain 8.08
Christopher Tomlinson Great_Britain 8.06

Reading from file and outputting to console through vector

2 Answers2