0

A student looking for some guidance...

This is a class assignment with the instructions: Re-write your program, List of Chores, using a unique_ptr object as your data member. You should store a dynamic array in the unique_ptr object. Use this array for storing, retrieving, deleting and updating chores.

I am trying to learn more about using the unique_ptr object so I may create the constructor which is supposed to initialize the list. A text file has the list and the constructor should store that list into an array. I am trying to work around the error "Access violation reading location." In the original program, I created a temporary dynamic array with a larger capacity and copied the list into that array. There's a list of 10 chores in the text file. Here is that code:

In Header:

private:
    /* var to keep len of list */
    int len = 0;
    int max = 9;
    /* add appropriate data structure to store list */
    string *arr = new string[max];

In .cpp:

/* reads the file line by line and initializes list */
ListOfChores::ListOfChores(string fileName){
    ifstream file(fileName, ifstream::in);
    string line;
        if (file.is_open()) //Checking if the file can be opened
        {
        while (getline(file, line)) // Gets a single line
        {
            if (len >= max)
            {
                string *narr = new string[max + 10]; // New, larger array
                for (int i = 0; i < max; i++)
                {
                    narr[i] = arr[i]; // Copies line
                }
                delete[] arr; // Clears
                arr = narr; // Copies
                max += 1; // Growth
            }
            arr[len] = line; // Store a line in the array
            len++; // Increases length by 1
        }
        file.close(); // Closes file
    }
    else cout << "Unable to open file" << endl;
}

And to show that I have been working on this and am not a lazy student...

New program attempt header:

private:
    /* var to keep len of list */
    int len = 0;
    int max = 9;
    /* add appropriate data structure to store list */
    string *arr = new string[max]; // Primary array
    string *narr = new string[max]; // New array

New program .cpp:

/* reads the file line by line and initializes list */
ListOfChores::ListOfChores(string fileName) {
    unique_ptr<string[]> arr(new string[max]); // Unique pointer initialization

    ifstream file(fileName, ifstream::in);
    string line = " ";
    if (file.is_open()) //Checking if the file can be opened
    {
        while (getline(file, line)) // Gets lines from file
        {
            if (len >= max)
            {
                max++; // Growth
                unique_ptr<string[]> narr(new string[max]); // New unique pointer
                narr = move(arr);// narr owns the object
                narr[max] = line; // Store a line in the array
                len++; // Increases length by 1
                arr = move(narr); // arr owns the object
            }
            else
            {
                arr[len] = line; // Store a line in the array
                len++; // Increases length by 1
            }
        }
        file.close(); // Closes file
    }
    else cout << "Unable to open file" << endl;
}
Rhuen
  • 23
  • 6
  • Recommendations: Rather than a flat 10, increase the size by 20-50 percent. Potentially more memory used, but significantly less copying. [Take a look at `std::copy`](http://en.cppreference.com/w/cpp/algorithm/copy) for your array copying needs. Don't keep remaking the `unique_pointer`; instead change the pointer managed by the `unique_ptr`. – user4581301 Sep 30 '16 at 06:00
  • Oh and don't declare the `unique_ptr` in the limited scope of your function. Make it a class member. The entire point of the `unique_ptr` is when it goes out of scope it deletes it's contents. If it is scoped within the function, the function ends and the data goes bye-bye. – user4581301 Sep 30 '16 at 06:03
  • I think this assignment just want you replace `string *arr` to `unique_ptr arr` (and modify other code to make it working) – apple apple Sep 30 '16 at 06:03
  • I'll try these recommendations in the morning, thanks :) – Rhuen Sep 30 '16 at 06:09
  • I agree with @apple, but most of all as we're talking about pointers/unique_ptr the exercise seems to be about memory management, but you haven't posted the destructor of your class and that has me worried. – n.caillou Sep 30 '16 at 06:14
  • Do the `unique_ptr` bit right and no custom destructor is required. – user4581301 Sep 30 '16 at 06:16

1 Answers1

0

The entire point of a unique_ptr is to destroy the managed pointer when the unique_ptr goes out of scope. This means you do not want it declared inside any confined scope. Any work you do will be destroyed as soon as the current block of code is exited. A unique_ptr created inside a function and not returned to the caller is gone and it's contents are gone with it as soon as the function returns. If you have a class intended to store a dynamic amount of data, then the data should be managed at the object's scope.

So

private:
    /* var to keep len of list */
    int len = 0;
    int max = 20; // made bigger so first 50% increase is 10 elements
    /* add appropriate data structure to store list */
    std::unique_ptr<std::string[]> arr; // Primary array
    // no need for New array here

Discussion of whether a 50% or 100% array size increase is better can be found here: What is the ideal growth rate for a dynamically allocated array?. As always, your mileage may vary, but a one-at-a-time increase is generally agreed upon to be a bad idea.

Now onto ListOfChores where we want to use, but not declare, the unique_ptr.

ListOfChores::ListOfChores(string fileName) {
    //no unique_ptr here. Scope is too narrow to be useful    
    ifstream file(fileName, ifstream::in);
    string line = " ";
    if (file.is_open()) //Checking if the file can be opened
    {
        while (getline(file, line)) // Gets lines from file
        {
            if (len >= max)// the current size is too small Let's make it bigger!
                           // if we grow before adding the line, we don't need any 
                           // special code to add the new line.
            {
                max *= 1.5; // Grow by 50%. Numerous studies have shown that 50% is a 
                            // good balance of RAM vs copy overhead in the general case
                std::string * narr = new string[max]; // no unique_ptr here either
                // old school copy for simplicity and obviousness
                for (int index = 0; index < len; index++)
                {
                     narr[index] = arr[index]
                } 
                arr.reset(narr); // frees and replaces the old array
                                 // arr now manages narr
            }
            // done growing, add normally to array 
            arr[len] = line; // Store a line in the array
            len++; // Increases length by 1
        }
        file.close(); // Closes file
    }
    else cout << "Unable to open file" << endl;
}

Other ListOfChores member functions will use arr, reading, adding and subtracting as needed. In order to add efficiently, the array growth code should be removed from the constructor and placed in a private method to be called by the constructor and other methods that need to enlarge the array.

Community
  • 1
  • 1
user4581301
  • 33,082
  • 7
  • 33
  • 54
  • I am still getting a thrown exception at `arr[len] = line;` : Exception thrown: read access violation. std::_String_alloc > >::_Myres(...) returned 0x18. – Rhuen Sep 30 '16 at 15:50
  • Also, `narr[index] = arr[index]` needs a semicolon haha – Rhuen Sep 30 '16 at 15:57
  • One piece I left out was the constructor. The constructor will need a `arr.reset(new string[max]);` to allocate and store the array in the first place. – user4581301 Sep 30 '16 at 15:58
  • Added the semicolon and placed `arr.reset(new string[max]);` at the very beginning of the constructor, works great. Thank you. – Rhuen Sep 30 '16 at 16:16