C++ OOP, problem with reading file, EOF used two times, leaderboard

Question

I am working on a little game and I want to make a leaderboard. I have class leaderboard and I am creating dynamic table, depending on how many players in the leaderboard.txt are. So that's one while eof loop. Then i want to asign names and points to these dynamic tables in leaderboard class. Problem is that i get random numbers instead of names and points. For me the code looks good. Any help?

class Leaderboard
{
    int max_counter;
    int counter;
    int *points;
    string *name;
    string filename;

public:
    Leaderboard(string n_file)
    {
        counter = 0;
        filename = n_file;
    }

string get_file(){return filename;}

void set_counter(int n_counter)
{
    max_counter = n_counter;
    points = new int[n_counter];
    name = new string[n_counter];
}

void add_value(string n_name, int n_points)
{
    name[counter] = n_name;
    points[counter] = n_points;
    counter++;
}

void show()
{
    for(int i=0;i<max_counter;i++)
    {
        cout << name[i] << " " << points[i] << endl;
    }
}

};

AND main:

Leaderboard *top = new Leaderboard("leaderboard.txt");
            fstream file;
            file.open(top->get_file(), ios::in);
            if(file.good())
            {
                string name;
                int points;
                int counter = 0;

                while(!(file.eof()))
                {
                    file >> name >> points;
                    counter++;
                }
                counter--;
                top->set_counter(counter);
                while(!(file.eof()))
                {
                    file >> name >> points;
                    top->add_value(name,points);
                }

                cout << "Dodano pomyslnie" << endl;
                system("pause");
                top->show();

                file.close();
            }
            else cout << "Blad z plikiem!" << endl;

            delete top;

            break;

See [this question](https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong) for why using `eof()` as a loop condition is almost always wrong. — François Andrieux, Mar 15 '19 at 20:46
Since the first loop only ends when the file reaches the end of the file, when you try to enter the next loop your stream is already at the end of the file. — François Andrieux, Mar 15 '19 at 20:46
@FrançoisAndrieux Thank you! That's clear for me now, seems like it's time to learn how to live without eof. — Danielix, Mar 15 '19 at 20:49
Ignoring EOF won't help you. If you try to read the file again from the same stream you will fail immediately because the stream has nothing left to read. You have to set the read position back to the beginning. Edit : See [this question](https://stackoverflow.com/questions/7681555/resetting-the-end-of-file-state-of-a-ifstream-object-in-c). — François Andrieux, Mar 15 '19 at 20:51
A sidenote: `int *points;` and `string *name;` make your job much harder than it needs to be. `Leaderboard` leaks memory because it has no destructor to clean up `points` and `name`, but as soon as you add the destructor, you'll find you're in violation of [The Rules of Three and Five](https://en.cppreference.com/w/cpp/language/rule_of_three). If you replace the dynamic arrays with [`std::vector`](https://en.cppreference.com/w/cpp/container/vector) you can observe the Rule of Zero because `std::vector` complies with the Rule of Five. — user4581301, Mar 15 '19 at 21:06

score 1 · Accepted Answer · answered Mar 15 '19 at 20:55

1

A couple of errors

            while(!(file.eof()))
            {
                file >> name >> points;
                counter++;
            }

should be

            while (file >> name >> points)
            {
                counter++;
            }

Second error, you can't expect the file to magically go back to the beginning just because you want it to. You have to tell it to.

            while (file >> name >> points)
            {
                ...
            }
            file.clear(); // clear error state
            file.seekg(0); // go to beginning of file
            while (file >> name >> points)
            {
                ...
            }

answered Mar 15 '19 at 20:55

john

85,011
4
57
81

Thank you, that's what i was looking for! – Danielix Mar 15 '19 at 22:19
I did like you adviced, but now nothing works. Process returned -1073741819 (0xC0000005) execution time : 2.337 s – Danielix Mar 16 '19 at 12:32
@Danielix Undoubtedly fixing one bug uncovers another. It's the reason you should only write a little code at a time, and make sure that code is fully working before writing any more. Most beginners try to write too much code in one go and can get into a situation where multiple bugs are interacting in a way that makes it hard to make progress. – john Mar 17 '19 at 19:13

score 0 · Answer 2 · answered Mar 15 '19 at 21:25

Allow me to suggest that the general approach you're using here is open to considerable improvement.

Right now, our main knows (and has to know) a great deal about the internals of the Leaderboard to do its job.

It would be better if that were not required. The Leaderboard itself should be the only part that knows about its internals.

Let me go a bit further though: a leaderboard is basically just a collection of scores. It shouldn't know or care about the internal details of an individual score either.

Finally, let me suggest that you consider using a container from the standard library. In your case, it appears that std::vector will work quite nicely.

#include <iostream>
#include <vector>
#include <iterator>
#include <vector>
#include <fstream>
#include <algorithm>

class score {
    std::string name;
    int points;
public:

    friend std::istream& operator>>(std::istream& is, score& s) {
        return is >> s.name >> s.points;
    }

    friend std::ostream& operator<<(std::ostream& os, score const& s) {
        return os << s.name << ": " << s.points;
    }
};

class leaderboard {
    std::vector<score> scores;
public:
    friend std::istream& operator>>(std::istream& is, leaderboard& l) {
        std::copy(
            std::istream_iterator<score>(is), std::istream_iterator<score>(),
            std::back_inserter(l.scores));
        return is;
    }

    friend std::ostream& operator<<(std::ostream& os, leaderboard const& l) {
        for (auto const& s : l.scores)
            os << s << "\n";
        return os;
    }
};

int main() {
    leaderboard scores;
    std::ifstream in("leaderboard.txt");

    in >> scores;

    std::cout << "Top scores\n";
    std::cout << scores;
}

Of course there's more that almost certainly should be done, such as sorting the scores in descending order by score, so the person with the top score shows up first--but that's kind of a separate issue.

Thank you! I will try this one, I really appreciate it. – Danielix Mar 15 '19 at 22:19 — Danielix, Mar 15 '19 at 22:19

C++ OOP, problem with reading file, EOF used two times, leaderboard

2 Answers2