|I'm forbidden to use STL and string in this task.|
Ah... of course. Acadamia loves to teach people the wrong way to do things.
|Why I don't create deep copy in first place? Because there are so many copies and just few of them are changed and they take so much memory.|
Oye... so you want to mix
deep and shallow copies? That's atrocious. If your class is telling you to do this, it is teaching you really bad habits.
All copies should be deep copies. If you want to have a shallow copy... don't copy it. Instead use a pointer to refer to the original object:
MyClass b = a; // should be a DEEP copy!
// don't want a deep copy? use a pointer!
MyClass* c = &a; // not a deep copy.
|I'm not leaking anything. Don't worry, everything is deleted through destructor. |
- Unless the code you posted is from the copy constructor, it leaks memory.
- If you have shallow copies and are deleting them in the destructor, you will cause your program to crash because you'll be deleting the same data multiple times.
You're in real ugly territory here, and the best way to do it right is to clearly define who owns what memory. This becomes very complicated when you "share" ownership (ie: shallow copy) because then memory has no clear owner and the task of cleaning it up becomes awkward and error prone.
1) Don't have anything do shallow copies. Ever. The closest thing to shallow copies you should ever do are "copy on write" semantics, but even that is usually not a good idea.
2) Memory should be owned and managed by whichever class defines it. In this case, since TRecord has a bunch of char*'s, then the TRecord class should be the one responsible for allocating/destroying that data:
struct TRecord // or you can make this a class with private members if you want encapsulation
// default ctor (to make sure the pointer is always valid
Name = new char;
strcpy( Name, "" );
// copy ctor (to deep copy)
TRecord(const TRecord& copy)
Name = new char[ strlen(copy.Name) + 1 ]; // thanks LowestOne for correction
strcpy( Name, copy.Name );
// assignment operator (to deep copy on assignment)
// note there are much better ways to implement this, I'm just showing this because
// it's the easiest way to illustrate. Again, the "best" way to implement this is to
// NOT implement it and let std::string worry about this crap.
TRecord& operator = (const TRecord& copy)
if(© == this) return *this; // to prevent self-assignment
delete Name; // old data must be deleted before we make it point to new data
Name = new char[ strlen(copy.Name) +1 ]; // thanks LowestOne for correction
strcpy( Name, copy.Name );
If you have that set up, the "owner" class which has the array of TRecords and can just copy the TRecords in full without having to copy each individual member.
Likewise, you should also do this with TPlace... so then TRecord does not have to be responsible for copying TPlace's members.
3) Don't allocate pointers to pointers just for a single object. This made me want to vomit:
m_record = new TRecord * [k*SIZE];
m_record[i] = new TRecord; // BARF!
There is no reason for m_record[i] to be a pointer here. You are just making it needlessly complicated and slowing it down by doing extra allocation and cleanup. Just make your array and array of objects instead of an array of pointers.
I also don't know why you are allocating
elements, but then only assigning
elements. This raises a few questions:
- What is k and why does it exist?
- What is the difference between SIZE and m_count?
Unless this is a size/capacity thing where you are overallocating in anticipation of future growth... but even then...
Anyway I went on a tangent here. The point is... allocate an array of object, not of pointers. What's more, since those objects all take care of their own copying, assigning them is easy:
m_count = copy.m_count;
k = copy.k;
m_record = new TRecord[k*SIZE]; // allocate objects, not pointers.
// m_record should be a TRecord*, not a TRecord**
for(int i = 0; i < k*SIZE; ++i)
m_record[i] = copy.m_record[i]; // let the class's assignment operator do the actual work