How does this dynamic object creation look like in terms of correctness or good code structure?

Question

This is a part of code for A* algorithm. I want to know if this is good, or I'd be better off using static allocation of memory here (I think that would be better). But not very sure as new to C++. Also, I'm using grid for the algorithm so how do I delete this. Please guide. Any suggestions welcome. Thanks.

for (int i = 0; i<world.size()-1 ; i++)
{
     for(int j =0; j<world[0].size()-1; j++)
     {           
         Node* a = new Node(make_pair(i,j), world[i][j]);
         grid[i].push_back(*a);
     }
}

Consider that whenever you explicitly use `new` there is probably something wrong with your code. — , Apr 05 '18 at 20:49
There is no point using dynamic allocation if you are going to copy its value and throw away the pointer. — Galik, Apr 05 '18 at 20:50
This might be better suited for [Code Review](https://codereview.stackexchange.com/tour) but it is definitely off-topic here. — Hermann Döppes, Apr 05 '18 at 20:52
Yes, I realize now that dynamic allocation is useful when unsure of the size or going to add more things to the heap. So, I should use : Node a(make_pair(i,j), world[i][j]); grid[i].push_back(a); — Nothing_8484, Apr 05 '18 at 20:54
Dynamic is also useful for objects with no fixed lifetime. That said, you almost always want to tie the lifetime to something. [Read up on RAII](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii) and the concept of ownership. — user4581301, Apr 05 '18 at 20:58

Tomáš Zahradníček · Accepted Answer · 2018-04-05T20:59:04.640

As you are pushing Node instead of Node* to the grid vector, you are copying value of *a (by copy/move constructor). Therefore the allocated Node will only be used for copying, and then never used.

You should either use local variable, and then you dont care for any deletion from your side

Node a = Node(make_pair(i,j), world[i][j]);
grid[i].push_back(a);

Or you can make grid a vector of pointers (vector<Node*>) and push the a itself. Then you have to manualy delete all pointers in vector the same way you did create them.

Node* a = new Node(make_pair(i,j), world[i][j]);
grid[i].push_back(a);

First case can be made better with emplace function, avoiding copying - constructing Node in the array of vector:

grid[i].emplace_back(make_pair(i,j), world[i][j]);

In second case, you could use some smart pointers, for example std::unique_ptr (vector>), so that you dont have to manually delete the pointers. Destructor of unique_pointer will handle it.

grid[i].push_back(make_unique<Node>(make_pair(i,j), world[i][j]));

score -1 · Answer 2 · answered Apr 05 '18 at 21:04

-1

A tip would be to store the address rather then object. A map would be a useful object to manage you nodes.

answered Apr 05 '18 at 21:04

Mike

190
7

How does this dynamic object creation look like in terms of correctness or good code structure?

2 Answers2