How to efficiently manage heap objects in array (C++)?

Question

I am having some issues with implementing a solution that deals with object creation on the heap in a loop and stores this in an array.

I have a transaction class that represents a share transaction. It includes custom date and time classes as well as some double values to store numeric values.

I read these using my TransactionIO class from a CSV file that contains around 100 transaction records.

My algorithm is as follows:

While EOF not reached
  Read Transaction data
  Create Transaction object on heap
  Return pointer to newly created Transaction object
  Store 'Pointed-to-Transaction' in custom Vector
EndWhile

Here is the code for the TransactionIO function ReadTransaction:

// Reads in a transaction and returns it
Transaction* TransactionIO::ReadTransaction(ifstream& is)
{
    // Assuming file structure is a CSV file
    // With structure given as:
    // Date/Time, Price, Volume, Value, Condition

    Date date;
    Time time;
    double price, volume, value;
    string condition;
    string dateString, timeString;

    //cout << "\nBegin Read..." << endl;

    getline(is, dateString, ',');       // read date/time string and then parse
    ReadNextDoubleField(price, is);
    ReadNextDoubleField(volume, is);
    ReadNextDoubleField(value, is);
    getline(is, condition);

    // split the date and time fields and parse them
    timeString = dateString.substr(dateString.find_first_of(" ") + 1);
    dateString.erase(dateString.find_first_of(" "));    // remove the time string - only need date string here

    date = ParseDate(dateString);    // Will change later to use pass by reference
    time = ParseTime(timeString);    // Will change later to use pass by reference

    // construct and return transaction that was read
    Transaction* transaction = new Transaction(date, time, price, volume, value, condition);
    return transaction
}

I have not created my main class yet, as I was working on my data classes. So what would be the correct way to use this function?

I am planning on doing this:

In the loop in main:

While(//file IO condition here...)
{
   p_transaction = TransactionIO::ReadTransaction(is);
   myCustomVector.Add(*transaction);
}

Is this the right way to do it? My custom vector's Add method expects a const T& reference to add the given object.

Also, will calling delete[] on my custom vector's internal array delete the objects being stored in it?

I feel my code is very inefficient and my lecturer has warned me to not construct objects in loops. I should return objects by reference.

But in this case If i tried that wouldn't this be invalid?

void TransactionIO::ReadTransaction(Transaction& transaction, ifstream& is)
{
   // Do all the reading and processing as given above....
   Transaction t1(date, time, price, volume, value, condition);
   transaction = t1;    // creating object AND call assignment op - not efficient?
}

In the above line, once this function completes, t1 will go out of scope and the object will destroyed. So what will my reference be pointing to?

Which is why I decided on the pointer solution, but I feel that my program will have a huge memory leak...

Any help and explanation would be highly appreciated... I like to know not just the 'how' but also the 'why' of your answer if possible.

And lastly, no. I cannot use STL containers. I have to use my own vector class (I have tested it out and it works well.)

Answer 1

To allocate the object in a loop is inefficient if it has a long run.

One thing that you can do is to pre-allocate your vector, but it can be inefficient too if you have no idea what the size you need. In STL you could use reserve: http://www.cplusplus.com/reference/vector/vector/reserve/ But in your case you need to implement something similar in your vector class.

In the above line, once this function completes, t1 will go out of scope and the object will destroyed. So what will my reference be pointing to? Undefined behavior, except if Transaction is some kind of smart pointer.

Also, will calling delete[] on my custom vector's internal array delete the objects being stored in it?

If I understood well, yes.

So you can reserve the size of the vector (if you've a good estimate size), and then populate the already created slots. This way your vector doesn't need to expand the size so frequently.

Answer 2

I would agree that you are better off stack-allocating your objects.

Heap allocation is comparatively expensive and in your case unnecessary as you are only using the pointers as temporary storage before copying your objects into the vector. You would then need to delete those pointers, which I don't see you doing.

In the case of this:

void TransactionIO::ReadTransaction(Transaction& transaction, ifstream& is)
{
   // Do all the reading and processing as given above....
   Transaction t1(date, time, price, volume, value, condition);
   transaction = t1;    // creating object AND call assignment op - not efficient?
}

You are not making transaction into a reference to t1, you are assigning the contents of t1 to the object which transaction is a reference to. You could even do transaction = std::move(t1); to explicitly use the move-assignment operator (so long as you have defined one or your class meets the rules for implicit definition) and eliminate the need for a copy.

However, you don't even need to pass in a reference, if you just return by value the compiler will elide the copy, instead allocating directly into the call site. This is known as Return Value Optimisation. As such, you can just do the following:

Transaction TransactionIO::ReadTransaction(ifstream& is)
{
   // Do all the reading and processing as given above....
   return Transaction(date, time, price, volume, value, condition);
}

Or in C++11:

Transaction TransactionIO::ReadTransaction(ifstream& is)
{
   // Do all the reading and processing as given above....
   return {date, time, price, volume, value, condition};
}