Does this cause a memory leak / How should I structure the code

I am slightly concerned that this code may have a memory leak. I would like to know if in fact there is a leak, and also what is the correct way to go about this problem.

Description: I have a base class Pet with derived classes Cat, Dog, and Bird. I am parsing lines from a file, and depending on certain contents in that line, I need to create an instance of the derived classes, and then parse part of the line again in a specific way. Here is a sample file:

Dog Spot Brown,Labrador,5
Cat Felix Black,7
Bird Polly Green,Parrot,12,Crackers

And some code:

class Pet
{
protected:
  string _type;
  string _name;
  string _desc;

public:
  Pet();
  bool ParseLine(std::string line);
  string Type() { return _type; }
  string Name() { return _name; }
  string Desc() { return _desc; }
};

class Dog : public Pet
{
private:
  string _color;
  string _type;
  int _age;

public:
  Dog(string type, string name, string desc);
  bool ParseDesc(string desc);
};

Main Code:

ifstream infile(filename, ifstream::in);
string line;
while(getline(infile, line))
{
   Pet* pet = new Pet();    // "new" called once
   if(pet->ParseLine(line))
   {
      if(pet->Type() == "Dog")
      {
         pet = new Dog(pet->Type(), pet->Name(), pet->Desc());   // "new" called again
         pet->ParseDesc(pet->Desc());
      }
      else if(pet->Type() == "Cat")
      {
         // ...
      }
   }
}

Basically what happens is this:

I take a line from the file and parse it into three fields (this is what ParseLine() does:

Type (Dog, Cat, Bird, etc.)
Name (Spot, Felix, Polly, etc.)
Description ("Brown,Labrador,5", "Black,7", "Green,Parrot,12,Crackers", etc)

I then assign these three fields to my Pet* variable.

Then, according to the value in Pet*->Type(), I parse Pet*->Desc() to get the additional information for that particular type of animal.

I am worried about calling operator "new" twice. I think there is probably a better way to format the code that could avoid this altogether.

I would really like to keep my getline() routine. I do NOT开发者_开发技巧 want to peek at the line to determine the type, and then decide how to create my instance.

Also, I have to reassign my variables _type, _name, and _desc, when I recreate the Dog(), and I'd rather not have to do that.

Thanks.

--

Specifically, how do I avoid this:

Pet* pet = new Pet();
pet->ParseLine(line);
string type = pet->Type();
string name = pet->Name();
string desc = pet->Desc();
delete pet;
if(type == "Dog")
{
   Pet* dog = new Dog(type, name, desc);
   dog->ParseDesc(desc);
}

Yes, this causes a memory leak, since you allocate a new Pet() which is never deleted and the pointer to it is overridden with either a new Dog() or something else.

I would suggest you create a so-called factory function, which reads a line from the file, and creates the type of Pet the line states.

Yes, there is a leak. If you do not want to release pointers manually, use some smart pointer library, e.g Boost shared_ptr.

As for how the leaks happen in your code: When you call new again and assign into the same pointer, you have a leak. When you leave the scope containing the pointer and do not release the memory, you have another leak.

However, your whole design smells to me and does not look right. You should not create a Pet only to realize later it is a Dog or a Cat and recreate it. You should have some "creator"(factory) object instead, which would handle this, and ParseLine would be a member of this factory object, not of the Dog or Pet.

In addition to other comments here, you need a virtual destructor in the base class to ensure derived classes are cleaned up properly when deleting via a Pet*.

virtual ~Pet() {}

See here for rationale.

Here is what I did in a similar situation. Note this is not the only or best way to do this.

I was reading random length records from a file where each record had a common fixed-sized header and the type of record/object would be determined from the header information. I created a factory-like class which read in the header, search a container for a matching entry and use a factory class/function to create the desired object. The header data was passed to the new object's constructor for copying.

In simple pseudo-code it was something like:

struct header_t
{
       int Type
       int Size
       ....
}

map<Type, CreateFunction> RecordFactory = 
{
      { TYPE1, CreateRecord1 },
      { TYPEX, CreateRecordX },
      ....
}

header_t NewHeader = ReadHeader()
RecordCreate = RecordFactory.Find(NewHeader.Type)
if (RecordCreate is valid) NewRecord = RecordCreate(NewHeader)

If you have a small, fixed number of classes then you don't necessarily need a complex factory class and a short if/switch list would work just as well. If you are not familiar with the Factory Pattern then read up on it as it is useful in a variety of situations.

I agree with comment about boost::shared_ptr, learn how to use it - it will make your life better

Looking at your '2 news' solution I now see what you are asking. THe ParseLine function should not be an instance method of Pet (A pet doesnt parse a line, a pet barks, eats, walks etc)

YOu should have a PetFactory class, that has a static method ParseLine and that returns a polymorphic Pet* (or better a PetPtr)

the rule of thumb is always to delete after you call new.
if(pet) delete pet;

For detecting memory leaks:-

1. There are tools available like purify and valgrind

2. When you do not have any access to these tools, there is a simple method Put the section of code you are doubtful of having a memory leak in infinite loop. Let the program run in that loop (for may be an overnight or so) If there is a memory leak, memory will get finished and new allocations will cease to happen. If you find it running smooth, enjoy there is not any memory leak.

Hope this helps.