CODEWITHSUNDEEP
c++arrayspointersstructure
cplusplusnewbie
cplusplusnewbie

Reputation: 1

Destructor of arrays of pointers in structures

I have this structures and dynamically allocated arrays. I can't use std::vector and std::string, because it is homework.

 struct Moves
    {
        const char* date;
        const char* street;
        const char* city;
    };

struct Data
{
    const char* id;
    const char* name;
    const char* surname;
    int count;
    Moves** moves;

};

I have a class, where I create a array of pointers on structure Data, where I dynamically allocate char* date, street, city.

Now, I need to delete these blocks of memory. Well, I've tried this: ( Destructor of my class) Question is: how should I free all allocated memory correctly?

class Reg
{
private:
 Data** arr;
 int counter;
public:
  Reg(){ arr=new Data*[1000]; }
  ~Reg();
  ... other methods
};

Reg::~Reg()
    {
        for(int i=0;i<counter;i++)
        {
            for(int c=0;c<arr[i]->count;c++)
            {
                delete arr[i]->moves;
            }

            delete arr[i];
        }

        delete [] arr;
    }

Here is an example of allocating:

arr[counter]=new Data; 
arr[counter]->id=new char[12];
arr[counter]->id=id;
arr[counter]->name=new char[strlen(name)+1];
arr[counter]->name=name;
arr[counter]->surname=new char[strlen(surname)+1];
arr[counter]->surname=surname;

arr[counter]->moves=new Moves*[100];
arr[counter]->moves[0]=new TMoves;

arr[counter]->moves[0]->city=new char[strlen(city)+1];
arr[counter]->moves[0]->city=city;

arr[counter]->moves[0]->date=new char[strlen(date)+1];
arr[counter]->moves[0]->date=date;

arr[counter]->moves[0]->street=new char[strlen(street)+1];
arr[counter]->moves[0]->street=street;

Upvotes: 0

Views: 1890

Answers (3)

Beta
Beta

Reputation: 99104

Rather than try to address every problem in this code, I'll tell you a principle that for some reason is never taught in programming courses: Start small and simple, add complexity a little at a time, test at every step and never add to code that doesn't work.

Look at this:

arr[counter]->moves[0]->city=new char[strlen(city)+1];
arr[counter]->moves[0]->city=city;

Even assuming that this Moves has been correctly constructed, you allocate memory with new and then immediately abandon it, causing a memory leak. Then if city (a variable with the same name as a member, not a good idea) is a pointer to a char[] on the heap, and if nothing bad is done with city for the rest of its lifetime, and if no other pointer to that array exists (or at least nothing bad is done with such a pointer) then this will not lead to undefined behavior. Do you feel that lucky?

Instead, consider this:

struct Moves
{
private:
  const char* city;

public:

  Moves()
  {
    city = NULL;
  }

  ~Moves()
  {
    if(city)
      delete [] city;
  }

  void setCity(const char ncity[])
  {
    if(city)
      delete [] city;
    char *temp = new char[strlen(ncity)+1];
    strcpy(temp, ncity);
    city = temp;
  }
};

...
arr[counter]->moves[0]->setCity(someCity);

Note that once setCity() is working correctly, invoking it from outside is clean, safe and simple. And once Moves is set up correctly, Data can be rewritten in a similar fashion, and then Reg.

And once you're used to this approach, you can learn to use std::string, and never muck around with char[] again.

Upvotes: 4

Aesthete
Aesthete

Reputation: 18848

How much nicer is this?

struct Moves
{
  std::string date;
  std::string street;
  std::string city;
};

struct Data
{
  Data() { moves.reserve(1000); }
  std::string id;
  std::string name;
  std::string surname;

  typedef std::unique_ptr<Moves> MovesPtr;
  typedef std::vector<MovesPtr> MovesList;
  MovesList moves;
};

Now you can add new Moves and they will be released on destruction of the Data object.

int main()
{
  Data d;
  d.moves.push_back(Data::MovesPtr(new Moves()));

  return 0;
}

The STL is nice, is there to help you, and you should use it.

Upvotes: 0

Tom
Tom

Reputation: 2389

It's hard to answer this post, but here are a few suggestions:

  • Consider using std::vector instead of arrays
  • Consider using std::string instead of const char *
  • Consider putting the destruction of the members of struct Moves and struct Data within those definitions

For example:

struct Moves
{
    const char* date;
    const char* street;
    const char* city;

    ~Moves () {
      delete [] date;
      ...
    }

};

Here's your class Reg using a std::vector to hold objects of type Data *:

class Reg
{
  private:
    std::vector<Data*> arr;
    int counter;  // this can probably be removed
  public:
    Reg() 
      :arr(1000, NULL) // initialize arr with 1000 NULL raw pointers
    {
    }

    ~Reg();
    ... other methods
};

A good C++ reference is cplusplus.com.

If you enhance your question you'll get some good answers (and learn a lot of C++) :-)

Upvotes: 1

Related Questions