Quote:Original post by jyk
I didn't look at all the code, but I see a potential problem here:env_cell* findCell(float x,float z){ env_cell * foundCell;
You probably want: env_cell * foundCell = 0;
As is, foundCell is unitialized, which means that if no cell is found, the function may return this (as intended), or it may return an invalid pointer (which could easily lead to your program crashing or terminating unexpectedly).
... Better yet, initialize foundCell
with this, and then return it unconditionally after the loop instead of checking it. Or even skip having that variable completely, and just return whatever is found when you find it. ;)
Also, don't store iterators as class members. You only need it inside the find function, so it should get declared there. This is just applying the usual principle of scoping variables as tightly as possible, but it's especially important with class instances to avoid wasting memory.
There are a few other subtle things you can do to clean up the code right away without having to learn the really advanced techniques. Please pay close attention to all the differences - in particular, the
lack of comments, which are made unnecessary by other changes:
class environmentCell { // ...public: std::vector<environmentCell*> adjacentCells; // returns a pointer to an adjacent cell surrounding the specified point. // If none of the adjacent cells qualify, return this. environmentCell* findCell(float x, float z) { typedef typename std::vector<environmentCell*>::iterator iterator; for (iterator it = adjacentCells.begin(), end = adjacentCells.end(); it != end; ++it) { environmentCell* cell = *it; if (x <= cell->right && x >= cell->left && z <= cell->top && z >= cell->bottom) { return cell; } } return this; }};
By the way, this comment is worrisome:
Quote: //adds a cell to the vector of cells in the map (only called during init)
You should almost never have "two-phase construction" (an 'init' function that is called after the constructor). Constructors are for the construction work. If, on the other hand, the init() function is called to
implement the constructor, it - and its helpers, in turn - should probably be private. But again, why is creating the object so complicated that you need to cut up the work into these sorts of functions? Maybe you actually need to cut up the class itself?