views:

422

answers:

5

Hi, I'm doing some assignment and got stuck at one point here. I am trying to write an list_add() function. The first functionality of it is to add values to the array. The second functionality for it is to increase the size of the array. So it works much like a vector. I dunno if I get it right though. What I tried is to create a new dynamic allocated array which is larger than the old one, and then copy over all the values to the new array.

Is it the right approach?

Here's the main body

int main()
{
    const int N = 7;

    //declaring dynamic array allocation
    int* list = new int[N];

    int used = 0, a_val;
    for(int i=0;i<11;i++)
    {
        list_add(list, used, N, i);
    }

    cout << endl << "Storlek: " << N << endl << endl;
    cout << "Printar listan " << endl;
    for(int i=0;i<used;i++)
    {
        cout << list[i] << ". ";
    }

}

Here's the function

bool list_add(int *list, int& space_used, int max_size, int value)
{

    if(max_size-space_used > 0)
    {
        *(list+(max_size-space_used-1)) = value;
        space_used++;
        return true;
    }
    else
    {
        cout << "Increasing size of array!" << endl;
        int new_max_size = space_used+1;
        delete [] list;
        int *list_new = new int[new_max_size];

        for(int i=0; i<new_max_size; i++)
        {
            list_new[i] = i;
            cout << list_new[i] << ". ";
        }
        cout << endl;
        space_used++;
        list = list_new;
        return false;
    }
}
A: 

I would worry about this line:

*(list+(max_size-space_used-1)) = value;

and this one:

list_new[i] = i;
Aaron
the last line I used it to check so I actually got the array copied. It won't be there when it works
starcorn
+1  A: 
George
this assumes size is incremented appropriately outside the loop
George
Ty, I'm going to try this out.
starcorn
+1  A: 

One problem that jumps out at me is that you're not changing the value of your list pointer outside of the scope of your list_add function. You should make some changes like...

bool list_add(int *list, int& space_used, int max_size, int value)

becomes

bool list_add(int **list, int& space_used, int max_size, int value)

and

list = list_new

becomes

*list = list_new

Otherwise I think you'll find that when you reallocate your list, after returning from list_add your list pointer will still point to the old location.

Parappa
A: 

There is a lot to be said for knowing how to solve problems, but this isn't one of them.

#include <vector>
#include <iostream>

int main() 
{
    std::vector<int> numbers;

    for (int i = 0; i < 11; i++) {
        numbers.push_back(i);
    }

    for (int i = 0; i < numbers.size(); i++) {
        std::cout << numbers[i] << ". ";
    }

    std::cout << "\n";
}

UPDATE: As shown above in my other answer his function contains at least four bugs in 16 lines. That is a bug for every four lines of code. And then there are the problems with the design of the code. For example the size of the array and the array itself should be together. You can't otherwise guarantee that the function works.

Two of the problems in the code (2,4) could be solved by using a struct containing the array pointer and the max_size of the data structure. That way you have to pass the two variables together.

Peter Stuifzand
+1  A: 

There four problems with the implementation of your code:

  1. It doesn't copy the elements of the list.
  2. It doesn't assign the value of new_list to the list variable in main
  3. It inserts values from the back to the front, instead of after the last value
  4. max_size doesn't get updated. It's easy to miss this, because you only increase the size of the array by one each time. That way it would need to allocate each time a value is added. If you increase the new size by more then one it will still reallocate every time.

The first problem can be fixed by changing the for loop in list_add so it makes a copy:

for (int i = 0; i < space_used; i++) {   // this also changed.
    list_new[i] = list[i];
    cout ...
}
// insert the new value (in the front?)
list_new[max_size-space_used-1] = value;     
delete [] list;         // Delete the list afterwards instead of earlier.

The second problem can by fixed by returning a pointer to the list. Change the main function to this:

for (int i = 0; i < 11; i++) {
    list = list_add(list, used, N, i); 
}

The third problem can be fixed by changing this line

list_new[max_size-space_used-1] = value;

to

list_new[space_used++] = value;

You should also remove the space_used++ after this.

To see the fourth problem you should change this line

int new_max_size = space_used+1;

to

int new_max_size = space_used+3;

It will still reallocate every time. It should however reallocate only two times.


This is the full code:

#include <iostream>
using std::cout;
using std::endl;

int* list_add(int *list, int& space_used, int& max_size, int value) {
    if (max_size - space_used > 0) {
        list[space_used++] = value;
        return list;
    }
    else {
        cout << "Increasing size of array!" << endl;
        int new_max_size = space_used+1;

        int *list_new = new int[new_max_size];

        for (int i = 0; i < space_used; i++) {
            list_new[i] = list[i];
            cout << list_new[i] << ". ";
        }
        cout << endl;

        list_new[space_used++] = value;
        max_size=new_max_size;

        delete [] list;
        return list_new;
    }
}

int main() {
    int N = 7;

    //declaring dynamic array allocation
    int* list = new int[N];

    int used = 0, a_val;

    for (int i = 0; i < 11; i++) {
        list=list_add(list, used, N, i);
    }

    cout << endl << "Storlek: " << N << endl << endl;
    cout << "Printar listan " << endl;

    for (int i = 0; i < used; i++) {
        cout << list[i] << ". ";
    }
}
Peter Stuifzand
this solution inspired me on how to think. thanks!
starcorn