C++

Forum

 
Using new and delete with pointers

alexyoung1992 (31)
Hi,

I am trying to use new and delete with some pointers and I have a couple of questions. I have a lot of code, and I'd rather not post it just because it would get confusing for me to explain everything, but basically here's what's going on.

I have an abstract base class, let's call this "cell", with several derived classes, let's say "Movable" and "Immovable" and "Empty".

I want to declare a grid of cells, and then cheat my way into putting objects of derived classes in the grid. Thus, I have a vector of a vector of cell pointers.
(vector<vector<cell*> > grid)

To initialize the grid, I use nested for loops (using i and j), like so:
grid[i][j] = new Empty;

Then, anytime I want to say, make that Empty cell into an Immovable cell, all I do is:
delete grid[i][j];
grid[i][j] = new Immovable;

So my question is, is this use of new and delete "legal/acceptable" because it seems that after I try to use it too much (as in keep switch the types of my cells) my program crashes. Hopefully this made sense, thanks in advance for your help!
Catfish (666)
Well from what you wrote, the approach looks fine. (Although it's an abuse of dynamic memory allocation.)

So your program crashes? Then make sure you always set the pointer you delete to NULL.
Also check if you aren't allocating more memory than you physically have.
You can do so by catching the bad_alloc exception.

1
2
3
4
5
6
7
8
9
10
 
#include <new>

try
{
    // evil code here
}
catch (std::bad_alloc &)
{
    std::cerr << "Memory problems!" << std::endl;
}


Edit: Also, I hope you remembered to delete everything before the program ends.
Last edited on
alexyoung1992 (31)
Ok, 4 things.

1. Catching bad_alloc makes sense, I don't know why I didn't think of trying that before, thanks.

2. What's useful about setting the pointer to NULL right after I delete it, if in the next line of code I assign it to a new object?

3. If the user exits my program manually, I call a method to delete everything, but if the program crashes or quits unexpectedly, I'm pretty sure nothing is deleted because I haven't implemented any of the destructors. I wasn't quite sure how to handle this, because my vector of vector of pointers is not a member variable in any of my classes. Any advice for going about that?

4. I kind of figured it was a bad use of dynamic memory, but I couldn't think of any other way to approach this, what would be a better way of doing this?

Thanks for your help!
Catfish (666)
2. It's just in case you delete the same pointer for a second time, it'll be harmless if set to NULL beforehand.

4. Judging by the way those subclass cell objects are named (Movable, Immovable, Empty) I'd suggest using just one generic Cell type with a flag in it, describing it.

Then you wouldn't need to mess with the memory, just change the flag and then the member functions should check for that.

Of course this isn't an OOP approach so maybe it's not what you should do.
Otherwise, your own method is fine I guess.

3. ... gotta catch 'em all. Exceptions I mean, in main(). If anything nasty rears its head, close your eyes by using the universal catch (...) and call your cleanup function inside it.
Last edited on
alexyoung1992 (31)
Ok so my program is still crashing so I'm going to give a little bit more info -

2. My entire program is based on moving cells around. Let me give you a little bit more info: I have to program a turn-by-turn version of the 1984 PC game, Beast. There are a series of immovable and movable blocks that a single player block has to navigate around. Meanwhile, there are beasts that try to eat the player. The player tries to defeat the beasts by squishing it between two blocks. Additionally, if a player has blocks on one side of him, and he moves to the other side, he pulls the said blocks with him. Alternatively, he also pushes blocks involuntarily when he moves.

Thus, every time the player moves, I delete that pointer and then set it equal to a new Empty and then delete the pointer to which the player wants to move, and then set it equal to a new Player. The same concept applies to the beast moving around.

Thus, basically every single choice the user makes involves a lot of new and delete, and I fell like that is just plain wrong.

I'm going to include all of my header files, let me know which implementations you would like to see.

main.cpp 
1
2
3
4
5
6
7
8
 
#include "game_manager.h"

using namespace std;

int main(){
    Game_manager game;
    game.play();
}


game_manager.h 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
#include "data.h"
#include <vector>
#include <ostream>

#ifndef GAME_MANAGER_H
#define	GAME_MANAGER_H

class Game_manager{
public:
    void play();
private:
    void print_main_menu();
    void print_player_menu();
    void generate_map(int i, int j, std::vector<std::vector<cell*> >&);
    void clear_map(std::vector<std::vector<cell*> >&);
    void print_grid(std::ostream&, std::vector<std::vector<cell*> >&);
    bool read_input(int&);
    bool read_input(char&);
    void get_random_coord(int &x, int &y, int height, int width);
    void run(std::vector<std::vector<cell*> >&);
};

#endif	/* GAME_MANAGER_H */


data.h 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
 
#include<ostream>

#ifndef DATA_H
#define	DATA_H

enum cell_type{player, beast, movable, immovable, empty};

class point{
public:
    const point& operator=(const point&);
    bool operator==(const point&) const;
    bool operator!=(const point&) const;
    
    point();
    point(int x, int y);
    
    int get_x() const;
    void set_x(int);
    int get_y() const;
    void set_y(int);
    
private:
    int X, Y;
};

class cell{
public:
    cell();
    cell(point&);
    virtual ~cell();
    
    point getCoord() const;
    void setCoord(point&);
    void setCoord(int, int);
    virtual cell_type getType() const =0;
    virtual void print(std::ostream &) const =0;
    
protected:
    point coord;
    cell_type type;
};

class Empty : public cell{
public:
    Empty();
    ~Empty();
    virtual cell_type getType() const;
    virtual void print(std::ostream &) const;
};

class Player : public cell{
public:
    Player();
    ~Player();
    virtual cell_type getType() const;
    virtual void print(std::ostream &) const;
};

class Beast : public cell{
public:
    Beast();
    ~Beast();
    virtual cell_type getType() const;
    virtual void print(std::ostream &) const;
};

class Movable : public cell{
public:
    Movable();
    ~Movable();
    virtual cell_type getType() const;
    virtual void print(std::ostream &) const;
};

class Immovable : public cell{
public:
    Immovable();
    ~Immovable();
    virtual cell_type getType() const;
    virtual void print(std::ostream &) const;
};

#endif	/* DATA_H */


player_manager.h 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
#include <vector>
#include "data.h"

#ifndef PLAYER_MANAGER_H
#define	PLAYER_MANAGER_H

class Player_manager{
public:
    void move_left(std::vector<std::vector<cell*> > &);
    void move_right(std::vector<std::vector<cell*> > &);
    void move_up(std::vector<std::vector<cell*> > &);
    void move_down(std::vector<std::vector<cell*> > &);
    void pull_left(std::vector<std::vector<cell*> > &);
    void pull_right(std::vector<std::vector<cell*> > &);
    void pull_up(std::vector<std::vector<cell*> > &);
    void pull_down(std::vector<std::vector<cell*> > &);
private:
    point get_player(std::vector<std::vector<cell*> > &);
    void print_error(cell_type);
    
    int x, y;
};

#endif	/* PLAYER_MANAGER_H */
alexyoung1992 (31)
And here are the most important functions:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
 
void Game_manager::play()
{
    vector<vector<cell*> > grid;
    int choice = -1;
    int height = 0, width = 0;
    
    do{
        do{
            print_main_menu();
        }while(!read_input(choice));
        switch(choice){
            case 1:{
                if(grid.empty()){
                    cout<<"\nPlease enter the height: ";
                    cin>>height;
                    cout<<"\nPlease enter the width: ";
                    cin>>width;
                    try{
                        generate_map(height, width, grid);
                    }catch(bad_alloc &){
                        cerr<<"Generate map caused a bad_alloc.";
                    }catch(...){
                        clear_map(grid);
                    }
                }
                try{
                    run(grid);
                }catch(bad_alloc &){
                    cerr<<"Run caused a bad_alloc.";
                }catch(...){
                    clear_map(grid);
                }
                break;
            }case 2:{
                ofstream fout;
                string name="";
                cout<<"\nPlease enter a name for the file (without file extension): ";
                cin>>name;
                cin.ignore(numeric_limits<streamsize>::max(), '\n');
                name+=".txt";
                fout.open(name.c_str());
                if(fout.fail())
                    cerr<<"\nERROR:Failed to create output file.\n\n";
                else
                    print_grid(fout, grid);
                break;
            }case 3:{
                // Not implemented yet
                break;
            }case 4:{
                // Not implemented yet
                break;
            }case 5:{
                // Not implemented yet
                break;
            }case 6:{
                // Not implemented yet
                break;
            }case 7:{
                // Not implemented yet
                break;
            }case 8:{
                clear_map(grid);
                break;
            }default:{
                cerr<<"\nERROR: Default was called.\n\n";
            }
        }
    }while(choice != 8);
    return;
}


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
 
void Game_manager::generate_map(int height, int width, vector<vector<cell*> > &grid)
{
    int num_immovables = (height*width)/100;
    int num_movables = (height*width)/4;
    int num_beasts = (height*width)/200;
    
    grid.resize(height);
    
    for(int i = 0; i<height; i++){
        grid[i].resize(width);
    }
    for(int i = 0; i<height; i++){ //Sets the coordinates for each cell
        for(int j = 0; j<width; j++){
            grid[i][j] = new Empty;
            point p(i,j);
            grid[i][j]->setCoord(p);
        }
    }
    for(int j = 0; j<grid[0].size(); j++){ //Changes the first row to immovable
        point p = grid[0][j]->getCoord();
        delete grid[0][j];
        grid[0][j] = NULL;
        grid[0][j] = new Immovable;
        grid[0][j]->setCoord(p);
    }
    for(int i = 0; i<grid.size(); i++){ //Changes the first and last cell in the row to immovable
        point p = grid[i][0]->getCoord();
        delete grid[i][0];
        grid[i][0] = NULL;
        grid[i][0] = new Immovable;
        grid[i][0]->setCoord(p);
        
        p = grid[i][grid[i].size()-1]->getCoord();
        delete grid[i][grid[i].size()-1];
        grid[i][grid[i].size()-1] = NULL;
        grid[i][grid[i].size()-1] = new Immovable;
        grid[i][grid[i].size()-1]->setCoord(p);
    }
    for(int j = 0; j<grid[grid.size()-1].size(); j++){ //Changes the last row to immovable
        point p = grid[grid.size()-1][j]->getCoord();
        delete grid[grid.size()-1][j];
        grid[grid.size()-1][j] = NULL;
        grid[grid.size()-1][j] = new Immovable;
        grid[grid.size()-1][j]->setCoord(p);
    }
    srand ( time(NULL) );
    for(int i=0; i < num_immovables; i++){ //Generates random immovable blocks
        int x, y;
        
        get_random_coord(x, y, height, width);
        if(grid[y][x]->getType() != empty)
            get_random_coord(x, y, height, width);
        delete grid[y][x];
        grid[y][x] = NULL;
        grid[y][x] = new Immovable;
        grid[y][x]->setCoord(x, y);
    }
//    for(int i=0; i < num_movables; i++){ //Generates random movable blocks
//        int x, y;
//        
//        get_random_coord(x, y, height, width);
//        if(grid[y][x]->getType() != empty)
//            get_random_coord(x, y, height, width);
//        delete grid[y][x];
//        GRID[y][x] = NULL;
//        grid[y][x] = new Movable;
//        grid[y][x]->setCoord(x, y);
//    }   
    for(int i=0; i < num_beasts; i++){ //Generates random beasts
        int x, y;
        get_random_coord(x, y, height, width);
        if(grid[y][x]->getType() != empty)
            get_random_coord(x, y, height, width);
        delete grid[y][x];
        grid[y][x] = NULL;
        grid[y][x] = new Beast;
        grid[y][x]->setCoord(x, y);
    }
    { //Generates a player
        int x, y;
        get_random_coord(x, y, height, width);
        if(grid[y][x]->getType() != empty)
            get_random_coord(x, y, height, width);
        delete grid[y][x];
        grid[y][x] = NULL;
        grid[y][x] = new Player;
        grid[y][x]->setCoord(x, y);
    }
}


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
 
void Game_manager::run(vector<vector<cell*> > &grid)
{
    Player_manager player;
    char choice = ' ';
    
    do{
        print_grid(cout, grid);
        do{
            print_player_menu();
        }while(!read_input(choice));
        switch(choice){
            case 'a':{
                player.move_left(grid);
                break;
            }case 'd':{
                player.move_right(grid);
                break;
            }case 'w':{
                player.move_up(grid);
                break;
            }case 's':{
                player.move_down(grid);
                break;
            }case 'j':{
                player.pull_left(grid);
                break;
            }case 'l':{
                player.pull_right(grid);
                break;
            }case 'i':{
                player.pull_up(grid);
                break;
            }case 'k':{
                player.pull_down(grid);
                break;
            }case 'x':{
                break;
            }default:{
                cerr<<"\nERROR: Default was called.\n\n";
            }
        }
    }while(choice != 'x');
}
Catfish (666)
I'd suggest a different approach.
In real life, if you have a grid and some blocks, what happens when you move a block from one cell of the grid to another?

Simple: the block will change position, but it'll be the same block. Also, the grid and its cells are imaginary.

So how about you build a Block object which keeps track of its own position?
When the player interacts with a block, the block will check if it can move where the player pushes it, and the player can't move unless the block does too. And if the player himself can't move, then he can't drag the block either.

If you want to stick to your current version, at least try to determine where exactly it crashes.
Even if you would've provided the full source code, I personally still couldn't have been bothered debugging your program for you.
coder777 (8444)
You can keep that grid approach, but instead of new/delete you may consider to just simply swap the pointer.

One reason for a crash could be that you're using an object while it's already deleted.

This grid[grid.size()-1].size() looks really error prone. What if 'grid.size()' is 0?
alexyoung1992 (31)
coder777, what do you mean exactly by swapping the pointer? Originally, instead of using new and delete, I just set the pointer to a reference of whatever kind of object I wanted it to be, but I thought that was inefficient and impractical.

Also, even though I deleted the object, I immediately set the pointer to a new object. Does that not work the way I think it does? I thought I was trying to access the new object, not the deleted object?

Catfish, I've uploaded my source files here:

https://docs.google.com/leaf?id=0B4PMUnW5YugRZTJiN2E4YTAtNWE1NC00YzgzLThmMDYtMWQzMmMwNjhjNzBj&hl=en_US

Also, I understand my method of having a physical grid is also impractical, but I was unsure of how to implement a virtual grid. Additionally, I initially tried to have each cell keep track of its own coordinate, but I ended up not using it that much.

Also, what would happen if just never deleted the objects, and kept using new, until the whole grid was destroyed?
Last edited on
alexyoung1992 (31)
Never mind, I've been a complete idiot this whole time. Now I understand what you meant by swap pointers, at least here's my version of it. Create one new object of each, a Player, Beast, Movable, and Immovable. Have a member variable point to each of these objects, and simply set the pointer inside of the grid to the member pointer. That makes SO much more sense!
coder777 (8444)
No, that's not what i meant. To move an item within the grid can be done by swapping. Means if the player wants to move from 1/1 to 1/2 you can do it the following way: std::swap(grid[1][1], grid[1][2]);

Simple, isn't it?

Create one new object of each, a Player, Beast, Movable, and Immovable. Have a member variable point to each of these objects, and simply set the pointer inside of the grid to the member pointer. That makes SO much more sense!
Then you might fill your grid just with cell_type and print it within a (rather small) switch(). You would not need any pointer at all. The above mentioned swap would work either way.
Topic archived. No new replies allowed.