-2

I am trying to loop through the array but Player numbers that have multiple lines are either not accumulating their second instance or getting strange numbers (the last one).

    PlayerHitsWalksOuts
   1     2   2   2
19   0   5   7
 2   0   5   7
18   4   2   0
 4   3   3   6
12   2   2   2
 7   0   0   9 // should be 7 0 0 6 , because two 7 0 0 3 lines in input file
 8   1   4   1
10   2   2   2
 3   3   3   6
11   6   0   0
17   4   2   0
 9   3   2   1

I am lost on why it's not working since I'm pretty sure I wrote the main() correctly. The class file is correct as well, I believe. When I set the index to 0, I get "Segmentation fault (core dumped)".

#include "Player.h"
#include <iostream>
#include <fstream>

int findNumber(const Player p[], int numPlayers, int playerNumber);
void displayArray(Player team[], int team_size);

int main()
{
    fstream fin("baseball.txt");
    const int LIST_LENGTH = 20;

    int number = 0,
        hits,
        walks,
        outs,
        playerIndex,
        index = -1,
        teamSize = 0;

    cout << "This program tracks a baseball player's number "
         << "and their\nnumber of hits, walks, and outs for "
         << "each games in a season.\n";

    Player team[LIST_LENGTH];

    while (!fin.eof())
    {
        fin >> number >> hits >> walks >> outs;

        playerIndex = findNumber(team, teamSize, number);

        if (playerIndex == -1)
        {
            teamSize++;
            index++;
            team[index].setNumber(number);
            team[index].setHits(hits);
            team[index].setWalks(walks);
            team[index].setOuts(outs);
        }
        else
        {
            team[index].setHits(hits + team[index].getHits());
            team[index].setWalks(walks + team[index].getWalks());
            team[index].setOuts(outs + team[index].getOuts());
        }

        displayArray(team, teamSize);

        fin.close();
    }
}

int findNumber(const Player p[], int numPlayers, int playerNumber)
{
    int i;
    for (i = 0; i < numPlayers; i++)
    {
        if (p[i].getNumber() == playerNumber)
            return i;
    }
    return -1;
}


void displayArray(Player team[], int team_size)
{
    cout << "\n\nPlayer\tHits\tWalks\tOuts\n"
         << "------\t----\t-----\t----\n";

    for (int i = 0; i < team_size; i++)
    {
        cout << team[i] << endl;
    }
}

player.h:

{#include "Player.h"
#include <iostream>
#include <iomanip>
using namespace std;

Player::Player()
    {
    Number = Hits = Walks = Outs = 0;
    }

int Player::getNumber() const
    {
    return Number;
    }

int Player::getHits() const
    {
    return Hits;
    }

int Player::getWalks() const
    {
    return Walks;
    }

int Player::getOuts() const
    {
    return Outs;
    }

void Player::setNumber(int n)
    {
    Number = n;
    }

void Player::setHits(int h)
    {
    Hits = h;
    }

void Player::setWalks(int w)
    {
    Walks = w;
    }

void Player::setOuts(int o)
    {
    Outs = o;
    }
const Player& Player::operator=(const Player & p)
    {
      if (this != &p)
    {
      Number = p.Number;
      Hits = p.Hits;
      Walks = p.Walks;
      Outs = p.Outs;
    }
      return *this;
    }
ostream& operator<<(ostream& out,  const Player & p)
    {
      out << setw(2) << p.Number << "\t" 
      << setw(2) << p.Hits << "\t" 
      << setw(2) << p.Walks << "\t"
      << setw(2) << p.Outs;
      return out;
    }
existence
  • 1
  • 2
  • 1
    `while (!fin.eof())` https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong – drescherjm Nov 26 '18 at 00:07
  • You appear to be missing a } in your code before the else {} although your formatting style is pretty bad so its hard to read. – drescherjm Nov 26 '18 at 00:08
  • accidentally deleted it when I was trying to format it. There *is* a } before the "else" in the file I compilled. – existence Nov 26 '18 at 00:14
  • To format correctly copy and paste your code then select it an use the {} button. – drescherjm Nov 26 '18 at 00:16
  • I didn't quite get it. Would you mind making it more clear in your question what exactly you want to accumulate? – Swordfish Nov 26 '18 at 00:17
  • I still don't understand why the array isn't updating at all or updating incorrectly for certain numbers? – existence Nov 26 '18 at 00:19
  • Did you mention my question above? And please get rid of that `using namespace std;` in `Player.h`. – Swordfish Nov 26 '18 at 00:20
  • There is a file with a list of different player numbers (no names, just numbers) - and the numbers represent the team. So for example, there might be a line with player of team "2" with a certain number of hits, outs. and walks, and later in the file, another player of team "2" with different stats. Need to use a Player class stored within an array, to update players with same number (team) and the respective data so that it is cummulative. – existence Nov 26 '18 at 00:26
  • 1
    `team[index].setHits(hits + team[index].getHits());` you want `playerIndex` remember that `index` could be -1 and is not the player that you just found. – drescherjm Nov 26 '18 at 00:31
  • When I set the index to anything other than -1, I get "Segmentation fault (core dumped)". – existence Nov 26 '18 at 00:41
  • 1
    The way we're communicating now is kinda pointless. The code you had in your question before I reformatted it to be readable would not even compile. I'd suggest you update your question with your current, complete code including Player.h. And might I ask why you don't use a std::map for the members of a team? – Swordfish Nov 26 '18 at 00:50
  • I don't see the point in adding the class info because it's 100% right. the problem is in my main function but I can't figure out what's going on. But I'll add it anyway. – existence Nov 26 '18 at 00:53
  • *100% right.* – `operator=()` ... get rid of it. your class only contains `int`s which are trivially copyable. Also your check for self-assignment (`if (this != &p)`) is more likely to help you shoot your foot in the future than help in any way: [What is wrong with “checking for self-assignment” and what does it mean?](https://stackoverflow.com/questions/12015156/what-is-wrong-with-checking-for-self-assignment-and-what-does-it-mean) – Swordfish Nov 26 '18 at 01:07
  • Ok so it seems like nobody can figure out why the team[index] isn't updating? I thought this was a basic concept? – existence Nov 26 '18 at 01:14
  • @existence Yes, We Can! – Swordfish Nov 26 '18 at 10:43
  • 1
    Please don't make more work for people by vandalizing your posts. By posting on the Stack Exchange (SE) network, you've granted a non-revocable right, under the [CC BY-SA 3.0 license](//creativecommons.org/licenses/by-sa/3.0), for SE to distribute that content (i.e. regardless of your future choices). By SE policy, the non-vandalized version of the post is the one which is distributed. Thus, any vandalism will be reverted. – K.Dᴀᴠɪs Nov 26 '18 at 20:51

1 Answers1

1

I won't say much about Why is iostream::eof inside a loop condition considered wrong? because it hardly matters in shown code. Use the input operation itself as condition for the loop:

while (fin >> number >> hits >> walks >> outs)
{

You close the input file in the loop that is supposed to read from it:

while ( /* ... */ )
{
    // ...

    playerIndex = findNumber(team, teamSize, number);

    if (playerIndex == -1)
    // ...

    displayArray(team, teamSize);
    fin.close();  // <<========================= here. Remove that.
}

You are confusing index with playerIndex:

    // here you try to find the player by its number in the team
    playerIndex = findNumber(team, teamSize, number);

    if (playerIndex == -1)
    {
        teamSize++;
        index++;
        team[index].setNumber(number); // index could be ok in that case,
        // ...                         // but why use an extra variable
    }                                  // when teamSize (before it gets in-
    else                               // crementet) is the same?
    {
        // here you should be using the index that you found with
        // findNumber() instead of index. So replace index with playerIndex
        team[index].setHits(hits + team[index].getHits());
        team[index].setWalks(walks + team[index].getWalks());
        team[index].setOuts(outs + team[index].getOuts());
    }

Your code could be quite a bit shorter and easier to read if you implemented operator+=, a constructor which takes values and maybe a stream extraction operator for your class Player:

#include <cstdlib>    // EXIT_FAILURE
#include <fstream>
#include <iostream>
#include <iomanip>
#include <algorithm>  // std::find, std::copy
#include <iterator>   // std::ostream_iterator

class Player {
    int Number = 0;
    int Hits = 0;
    int Walks = 0;
    int Outs = 0;
public:
    Player() = default;

    Player(int number, int hits, int walks, int outs)
    : Number {number},
      Hits   {hits},
      Walks  {walks},
      Outs   {outs}
    {}

    Player& operator+=(Player const &other)
    {
        Hits  += other.Hits;
        Walks += other.Walks;
        Outs  += other.Outs;
        return *this;
    }

    int getNumber() const { return Number; }

    friend std::istream& operator>>(std::istream& in, Player &p)
    {
        int number, hits, walks, outs;
        if (in >> number >> hits >> walks >> outs)
            p = Player{ number, hits, walks, outs };
        return in;
    }

    friend std::ostream& operator<<(std::ostream& out, Player const &p)
    {
        return out << std::setw(2) << p.Number << "\t" << std::setw(2) << p.Hits << "\t"
                   << std::setw(2) << p.Walks << "\t" << std::setw(2) << p.Outs;
    }
};

bool operator==(Player const &lhs, int Number) noexcept
{
    return lhs.getNumber() == Number;
}

std::size_t findNumber(Player const *team, std::size_t numPlayers, int playerNumber)
{
    return std::find(team, team + numPlayers, playerNumber) - team;
}

void displayArray(Player *team, std::size_t numPlayers)
{
    std::cout << "\n\nPlayer\tHits\tWalks\tOuts\n------\t----\t-----\t----\n";
    std::copy(team, team + numPlayers, std::ostream_iterator<Player>{ std::cout , "\n" });
}

int main()
{   
    constexpr std::size_t LIST_LENGTH{ 20 };
    Player team[LIST_LENGTH];
    std::size_t teamSize{};

    std::ifstream fin("test.txt");
    if (!fin.is_open()) {
        std::cerr << "Couldn't open the players list for reading :(\n\n";
        return EXIT_FAILURE;
    }

    Player p;
    while (teamSize < LIST_LENGTH && fin >> p)
    {
        auto playerIndex{ findNumber(team, teamSize, p.getNumber()) };

        if (playerIndex == teamSize)
             team[teamSize++ ]  = p;
        else team[playerIndex] += p;
    }

    displayArray(team, teamSize);
}
Swordfish
  • 12,971
  • 3
  • 21
  • 43
  • I added playerIndex to both, and that seemed to give me the right answers, EXCEPT for the 7 team- for some reason it gave me 7009 instead of 7006, when the file has two 7003's and therefore should have given me a "6". So I'm not sure if the problem actually fixed? – existence Nov 26 '18 at 02:07
  • *I added playerIndex to both* – Which "both"? – *7009 instead of 7006, when the file has two 7003* [...] given me a "6" – Sorry, but i have no idea what you are talking about. Why not add you input file to your question so I can see for myself? – Swordfish Nov 26 '18 at 02:14
  • EDIT: Put updates above. But In the original output I wrote above, for Player 7 it is showing that it is 7009 when it should be 7006, because in baseball.txt (the input file) there are two lines for player 7, both have 7003 (meaning player =7, hits = 0, walks = 0, and outs =3). So for some reason it is adding up player 7's wrong. Does that help? – existence Nov 26 '18 at 02:24
  • In the file you have shown in your question is **one** line with player number 7 which has 6 outs. Plan-B: give me a pastebin link and I'll look over it. – Swordfish Nov 26 '18 at 02:27
  • yeah your edits helped get me to the point where it is all right, except for that one line, the 7 line, for whatever reason isn't reading. The input file has two 7 players, each with the same stats, 0, 0, and 3. But instead the "out" is showing as "9" not 6 on mine. Here is what I currently have. https://pastebin.com/1e6z9Ez6 – existence Nov 26 '18 at 02:34
  • You still `while(!foobar.eof())` ... See the first 4 lines of my answer. – Swordfish Nov 26 '18 at 02:41
  • It worked! But why??? I don't get how that would have effected the 6... – existence Nov 26 '18 at 02:56
  • Please. Go and read the Q&A I linked in my answer, and also @drescherjm pointed you at in the first comment to your question over three hours ago: [Why is iostream::eof inside a loop condition considered wrong?](https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong) – Swordfish Nov 26 '18 at 03:10