Fatal error from incrementing loop.

Question

Currently, I'm trying to make a function that sorts a vector full of fighters in the function sortFighters in the Fighter.cpp file. It all seems to compile correctly; However, when it does run, I get a fatal error in one of the lines of the aformentioned .cpp file. I know exactly what the problem is, and put a comment there accordingly. So, what I'm asking here, is what I might do to fix this problem without adding any other functions and such.

Here's my Fighter.h file:

#ifndef FIGHTER_H
#define FIGHTER_H

#include <iostream>
#include <ctime>
#include <string>
#include <cstdlib>
#include <fstream>
#include <vector>

class Fighter
{   
protected:
        std::string name;
        int health, level;
        //int damage;
public: 
        int  getHealth(int);
        void getEnemies(std::vector<Fighter> &);
        void printFighter(std::vector<Fighter> &);
        void sortFighters(std::vector<Fighter> &);
        //friend std::istream & operator >> (std::istream & strm, Fighter & x);
        //friend std::ostream & operator << (std::ostream & strm, const Fighter & f);
        //void attack();
        Fighter();
        ~Fighter();
};

class Player : public Fighter 
{ 
    private:
        int experience;
    public:
        int  getHealth(int);
        void pri`enter code here`ntFighter();
        void getExperience(int);
        void playerAttack();    
        Player();
        ~Player();
};

//class FightPub
//{
//  private:
//      Player player;
//      Fighter enemy;
//  public:
//      //void fight();
//      //void getStats();
//};
#endif

My Fighter.cpp file:

//dynamically locate an array that holds the number of fighters, and for each fighter in the array, assign from the .txt 
//file the name and level from the fighter.
#include "Fighter.h"  

#pragma region getEnemies
void Fighter::getEnemies(std::vector<Fighter> &baddie)
{
    Fighter x;
    std::ifstream inputFile;
    inputFile.open("EnemyFighters.txt");
    if(!inputFile)
    {
        std::cout << "error!" << std::endl;
    }
    else
    {
        while(!inputFile.eof())
        {
            std::string line;
            inputFile >> line;
            if (line == "<fighter>")
            {
                do
                {
                    inputFile >> line;
                    x.name = line;
                    inputFile >> line;
                    x.level = atoi(line.c_str());
                    inputFile >> line;
                    x.health = getHealth(this->level);
                    baddie.push_back(x);
                    inputFile >> line;
                }while(line != "</fighter>");
            }                   
        }
        inputFile.close();
    }
}
#pragma endregion

#pragma region getHealth

int Fighter::getHealth(int lv)
{
    if(lv >= 6)
    {
        std::cout << "\nHealth Bonus!";
        this->health = lv * 2;
    }
    /*else if (lv > 1)
        for (int i = 1; i < lv; i++)
        {this->health += 2;}*/
    return health;
}

#pragma endregion

#pragma region attack
//void Fighter::attack()
//{
//  int randomAttack = rand() % 4 + 1;
//
//  switch (randomAttack)
//  case 1: 
//  {
//      std::cout << "Enemy uses critical attack!"
//  }
//}
#pragma endregion

#pragma region printFighter
void Fighter::printFighter(std::vector<Fighter> &baddie)
{
    //std::cout << this;
    for (int i=0; i<baddie.size(); i++)
    {
        std::cout << "\nName: " << baddie[i].name << std::endl
                  << "Level: " << baddie[i].level << std::endl
                  << "Health: " << baddie[i].health << std::endl;
    }
}
#pragma endregion

void Fighter::sortFighters(std::vector<Fighter> &x)
{
    Fighter * temp = new Fighter;
    bool swap;

    do
    {
        swap = false;
        std::cout << x.size() << std::endl;
        for (int i=0; i<=(x.size()); i++)
        {
            //if the level in the first is greater than the level in the next
            if(x[i].level > x[i+1].level)//I get a fatal error here when it tries to compare 
                                         //the iterator with 1 that's outside its range
            {
                //assign the stats from the first to temp
                temp->name = x[i].name;
                temp->health = x[i].health;
                temp->level = x[i].level;
                //assign the stats from the next to the first
                x[i].name = x[i+1].name;
                x[i].health = x[i+1].health;
                x[i].level = x[i+1].level;
                //assign the ones in temp(the first) to the next
                x[i+1].name = temp->name;
                x[i+1].health = temp->health;
                x[i+1].level = temp->level;
                swap = true;
            }

            else if(x[i].level >= x[i+1].level)
            {
                temp->name = x[i].name;
                temp->health = x[i].health;
                temp->level = x[i].level;

                x[i].name = x[i+1].name;
                x[i].health = x[i+1].health;
                x[i].level = x[i+1].level;

                x[i+1].name = temp->name;
                x[i+1].health = temp->health;
                x[i+1].level = temp->level;
                swap = true;
            }

            else if (x[i].level < x[i+1].level)
            {
                //temp->name = x[i].name;
                //temp->health = x[i].health;
                //temp->level = x[i].level;

                //x[i].name = x[i+1].name;
                //x[i].health = x[i+1].health;
                //x[i].level = x[i+1].level;

                //x[i+1].name = temp->name;
                //x[i+1].health = temp->health;
                //x[i+1].level = temp->level;
                swap = false;
            }

            else if(x[i].level <= x[i+1].level)
            {
                /*temp->name = x[i].name;
                temp->health = x[i].health;
                temp->level = x[i].level;

                x[i].name = x[i+1].name;
                x[i].health = x[i+1].health;
                x[i].level = x[i+1].level;

                x[i+1].name = temp->name;
                x[i+1].health = temp->health;
                x[i+1].level = temp->level;*/
                swap = false;
            }
        }
    }while (swap);

    delete temp;
}
//std::istream & operator >>(std::istream & strm, Fighter x)
//{
//  //x.name += strm.c_str();
//  //x.level += atoi(strm.c_str());
//  strm >> x.name;
//  strm >> x.level;
//  return strm;
//}

//std::ostream & operator << (std::ostream & strm, const Fighter f)
//{
//  strm << "Name: " << f.name << std::endl;
//  strm << "Level: " << f.level << std::endl;
//  strm << "Health: " << f.health << std::endl;
//  return strm;
//}
#pragma region Fighter C&D
Fighter::Fighter()
{
    level = 1;
    health = 10;
}
Fighter::~Fighter()
{
}
#pragma endregion
//void operator <()
//{
//}
//
//void operator >()
//{
//}
//
//void operator <=()
//{
//}
//
//void operator >=()
//{
//}
//
//
//
int Player::getHealth(int lv)
{
    if(lv >= 6)
    {
        std::cout << "\nHealth Bonus!";
        this->health = lv * 2;
    }
    /*else if (lv > 1)
        for (int i = 1; i < lv; i++)
        {this->health += 2;}*/
    return health;
}

void Player::printFighter()
{
//std::cout << this;
      std::cout << "\nPlayer's stats: \n"
      << "Level: " << this->level << std::endl
      << "Health: " << this->health << std::endl
      << "Experience: " << this->experience <<std::endl;
}

void Player::getExperience(int dmg)
{
    experience += dmg;
    if (experience >= (level * 10))
    {
        std::cout << "Congratulations, Player! You're up a level!\n";
        level ++;
    }
}

#pragma region Player C&D
Player::Player()
{
    level = 1;
    health  = getHealth(level);
    experience = 0;
}
Player::~Player()
{
}
#pragma endregion 


//Player::printFighter()
//{
//  
//}

And here's main.cpp:

#include "Fighter.h"

int main()
{   
    unsigned seed = time(0);
    srand(seed);

    std::vector<Fighter> baddie;

    Fighter * enemy = new Fighter;
    Player * me = new Player;
    enemy->getEnemies(baddie);
    enemy->sortFighters(baddie);
    enemy->printFighter(baddie);
    me->printFighter();
    delete enemy;
    delete me;
    return 0;
}

Answer 1

    for (int i=0; i<=(x.size()); i++) 
    { 
        if(x[i].level > x[i+1].level)
        {

um.. Size() counts from 1. Indexes count from 0. So you'll want to make that i < x.size(), not <=. But, in the very next line, you say x[i+1], so i can't even reach the last item, it has to stop one before that:

    for (int i=0; i < x.size()-1; i++) 

Answer 2

Try Changing your for loop to this

for (int i=0; i < x.size() - 1; i++) {
  ... your original content...
}

This way x[i+1] never goes out of bounds
You were trying to access memory that has not been allocated.

Answer 3

Your problem is with this loop:

for (int i=0; i<=(x.size()); i++){ 
    if(x[i].level > x[i+1].level){  //Fatal Error Here 
        // Do some stuff
    }
    // Do some more stuff
}

Your condition for terminating the outer loop is i<=(x.size()) this means that when you do the comparision ( x[i+1].level ) that breaks the program, you are comparing outside the bounds of x. Because if i == x.size() then x[i+1] > x.size()

I reccomend changing your loop to terminate at i<(x.size())-1; rather than i<=(x.size());

Answer 4

The problem, as others have said, is this section of code:

    for (int i=0; i<=(x.size()); i++)
    {
        //if the level in the first is greater than the level in the next
        if(x[i].level > x[i+1].level)//I get a fatal error here when it tries to compare 
                                     //the iterator with 1 that's outside its range

A std::vector can be indexed by values 0 through size() - 1, inclusive. So, for a basic vector loop, you should instead do

for (int i = 0; i < x.size(); i++) {

BUT, in the next line of code, you check element i + 1, so you should instead do

for (int i = 0; i < x.size() - 1; i++) {

BUT, because x.size() is unsigned, if x is empty, then x.size() - 1 will be a very large number (2^32 - 1 on a 32-bit machine). So you should adjust the logic for this. Also, comparing signed values (like int i against unsigned values (like x.size() - 1) may generate compiler warnings (you should turn all such compiler warnings on, if you haven't already done so), so change i to unsigned or size_t:

for (size_t i = 0; i + 1 < x.size(); i++) {

Finally, rather than coding your own bubble sort, it's better to use std::sort. std::sort is faster and more familiar to other C++ developers. Here's an (untested) example of using std::sort:

bool CompareByLevel(const Fighter& a, const Fighter& b) {
    return a.level < b.level;
}

sort(x.begin(), x.end(), CompareByLevel);

Other comments: If you're interested in learning more about C++, here are a few other comments that might help.

inputFile >> line;

This statement actually reads a single whitespace- or newline-separated word into line. If that's what you're trying to do, then your choice of variable name (line) doesn't communicate that. And you may want to be able to use whitespace in fighters' names. To read an entire line, use this: (See here for reference.)

getline(inputFile, line);

getEnemies, printFighter, and sortFighters should be static methods, since they don't need a particular instance of a Fighter to operate on. (Right now, because they're not static methods, you have to create an enemy instance of Fighter in order to call those methods, even though none of those methods do anything with enemy.)

C++ lets you create variables on the stack, instead of dynamically allocating them. In other words, instead of saying

Fighter * temp = new Fighter;

just say

Fighter temp;

This is faster and simpler, if you don't need dynamic allocation.

Finally, C++ will create assignment operators for you that copy all of a class's members. So you could simply write

temp = x[i];

instead of manually assigning each of x's members to temp. Using the assignment operator is more robust, since it will continue to work in the future if you later add members to Fighter.

Hope this helps.

Answer 5

You've gotten some advice about how to fix that loop. My advice would be to eliminate it and use std::sort instead.

While we're dealing with loops that don't terminate correctly, however, it's probably also worth mentioning one other (in getEnemies()):

    while(!inputFile.eof())
    {
        std::string line;
        inputFile >> line;
        if (line == "<fighter>")
        {
    // ...

This is broken as well. For the loop to terminate correctly, you need to read the data, and then check whether the read succeeded:

std::string line;

while (inputFile >> line) {
    if (line == "<fighter>") {
         // ...

It's probably also worth noting that this code is pretty fragile -- just for one example, something like: <fighter>fighter1</fighter> will not be read correctly (it requires at least one whitespace before and after <fighter> to read it as a single string).