Conditional jump depends on unitialized value Valgrind

[Paul McKenzie]

Hi, I am really lost here, I have airport class which should navigate planes, in its list to runways, with method move, theres a method prepare which changes the direction of flight to all planes, always before move is called, move just increments decrement x and y of plane in its list. But after calling two times in row airport->move(), I get screwed and I really dont know wheres the problem. Have I badly initiazed something? Iterator gets invalidated.

1) None of this code needs dynamic allocation. A

std::list<plane>

is all that is needed. Once you do that, then all of those calls to "new" and "delete" goes away. You are not deriving plane from a base class, so there is no reason to be dynamically creating and using plane*.

2) As Philip pointed out, you do not need a copy constructor, since all the members of plane are safely copyable by themselves. As a matter of fact, your copy constructor is faulty:

/**
  Copy constructor
  @param other Airplane to make copy of
  */
plane::plane(const plane& other)
{
    _name=other.getName();
    other.getPosition(_x,_y);
    _direction=other.getDirection();
    _hasLanded = other._hasLanded;
    previous = _direction;
}

Look at the line in red. Why aren't you copying "previous" from other.previous? This is a bogus copy constructor, as there is no guarantee that this is an exact copy of other. This is why you don't write copy constructors if you don't have to -- let the compiler do the copying, as it will not make a mistake like the one you made above.

3) Where is your assignment operator to go along with the copy constructor? Better yet, don't write an assignment operator, since the compiler generated one will work OK without you writing it, the same way that the compiler generated copy constructor would have worked perfectly without you writing one.

4) Learn to use algorithm functions such as std::find_if() to search for particular planes, and std::for_each() to apply an action to every plane.

5) Don't start your member names with underscores. Names with underscores are reserved for the compiler's usage. Even if technically there is no problem with your code now, it is always a good thing to take precaution and not start names with underscores.

6) Learn to use initializer lists when creating your classes. Instead of this:

plane::plane(const std::string& name, int x, int y, DIRECTION direction)
{
    _name=name;
    _x=x;
    _y=y;
    _direction=direction;
    previous = direction;
}

Use this:

plane::plane(const std::string& name, int x, int y, DIRECTION direction) :
    name_(name),  x_(x), y_(y), direction_(direction), previous_(direction)
{ }

This is assuming you change the member names to have underscores after the name instead of starting a name.

7) Where is your "airport" class?

Given all of the above points, here is a sample of an addPlane() that uses these concepts:

#include <list>
#include <algorithm>

class plane
{
   // all of planes members here
};

typedef std::list<plane> PlaneList;

class airport
{
    PlaneList  planes;
    public:
        bool addPlane(const plane& p);
};

// a function object that gets called to compare two plane names and returns true
// if the names are equal, false otherwise
struct FindPlaneByName
{   
     FindPlaneByName(const plane& pl) : thePlane(pl) {}

     bool operator()(const plane& p) 
     { return p.getName() == thePlane.getName(); }
};

bool airport::addPlane(const plane& p)
{
    //  use find_if to search for a plane by name
    if ( std::find_if(planes.begin(), planes.end(), FindPlaneByName(p)) == planes.end() )
    {
        // plane not found, so add it to list
        planes.push_back(p);
        return true;
   }
   return false;
}

Note how much shorter and to the point this looks compared to your "addPlane" function. Less lines, and each line of code is worded to actually mean what it does. The "find_if" finds a plane given a certain criteria. That criteria is the name, so a function object that gets called by find_if() is written that compares by name. Maybe a little advanced, but might as well see now how it can be done without waiting around and maybe never seeing it for yourself.

As to your plane::move() function: Where are the parameters to move the plane a certain distance? Why is it not this?

void plane::move(int xDirection, int yDirection)
{
//
}

Maybe you should explain a little better how this "plane::move" with no arguments or criteria as to how much to move the plane is supposed to work. In any event, the airport::move() would just move each plane in the list.

Regards,

Paul McKenzie