Array is not resizing with operator new

Question

I want to implement a dynamic array as a class.

I haven written a method which adds an element at the end of the array:

void DynamicArray::addElementAtEnd() {
    cout << "\nPodaj liczbe calkowita: ";
    int* number = new int;
    cin >> *number;

    if (DynamicArray::array == NULL) {
        DynamicArray::array = new int[1];
        DynamicArray::array[0] = *number;
        delete number;
        (*DynamicArray::size)++;
        return;
    }
    int* buff = new int[*DynamicArray::size + 1];
    memcpy(buff, DynamicArray::array, (*DynamicArray::size) * sizeof(int));
    delete[] DynamicArray::array;
    buff[(*DynamicArray::size)] = *number;
    DynamicArray::array = buff;
    (*DynamicArray::size)++;
    delete number;
    return;
};

Here's the .h file of the DynamicArray class:

#include <iostream>
using namespace std;

class DynamicArray {
public:
    int* array;
    int* size;
public:
    DynamicArray() {
        DynamicArray::size = new int;
        *DynamicArray::size = 0;
    };
    void handleMenu();
    void readFromFile();
    void addElementAtEnd();
    void addElementAtBeginning();
    void addAtIndex(int index);
    void deleteElementAtEnd();
    void deleteElementAtBeginning();
    void deleteElementByIndex(int index);
    void showAllElements();
    void showElementAtIndex(int index);
    void findElementByValue(int value);
};

The problem is that this method adds only the first element, but if I try to add more then nothing happens. I debugged it, and the problem starts on this line:

int* buff = new int[*DynamicArray::size + 1];

I don't know why, but it seems like this line is not creating a bigger array.

I searched for some solutions, and it seems that the problem is connected with using *DynamicArray::size + 1 instead of eg a variable, or I don't do something right with it.

Size and the inserted number don't have to be (and shouldn't be) dynamically allocated. Only the array itself needs to be. — HolyBlackCat, Mar 07 '22 at 18:50
Use a `std::vector` instead of reinventing the wheel for gods sake. — πάντα ῥεῖ, Mar 07 '22 at 18:51
`int* number = new int;` is a troubling way to create a single function-local `int`. I genuinely wonder if there is some instructional source that suggests making every variable a raw pointer. — Drew Dormann, Mar 07 '22 at 18:55
There are some poor design choices in the given code, but it looks like it should function as expected. A lot of stuff could be going wrong outside the given code, ignoring the [Rule of Three](https://en.cppreference.com/w/cpp/language/rule_of_three) for example. — user4581301, Mar 07 '22 at 18:55
The indeterminate value of the `array` member after construction isn't doing you any favors either. Makes those future checks against `DynamicArray::array == NULL` a nice little (admittedly one-sided) gamble. Anyway, I *strongly* suggest consideration of the [single-responsibility principle](https://en.wikipedia.org/wiki/Single-responsibility_principle). I mean, does a dynamic array *really* need user-input iteration? does it *really* need a *menu* function ? big things are achieved by solving small problems, mate. — WhozCraig, Mar 07 '22 at 18:56
`DynamicArray::array` is not initialized. Your code expects it to be `NULL` at certain times, but you never assign `NULL` to that variable. Reading it before writing to it exhibits undefined behavior. If you correctly initialize that variable, your code works. — Drew Dormann, Mar 07 '22 at 19:07
You don't need to prefix members with `DynamicArray::` inside a member function. It only makes the code difficult to read. — molbdnilo, Mar 07 '22 at 19:40
Add `DynamicArray(DynamicArray const&) = delete;` and `DynamicArray operator=(DynamicArray const&) = delete;`, because the compiler synthesized ones won't do the right thing. Also add a `~ DynamicArray` destructor to clean up properly. — Eljay, Mar 07 '22 at 19:52

KyreX · Answer 1 · 2022-03-07T21:57:42.580

The actual problem is that you are not initializing array to NULL. So when you check if array is NULL on the first iteration, it is often not.

The minimal solution:

DynamicArray::DynamicArray() {
  this->size = 0; // You should use 'size' like an int, not a pointer
  this->array = NULL;
}
// Or using the Member Initializer List (by @user4581301)
DynamicArray::DynamicArray(): size(0), array(nullptr) {}

Note: Differences between NULL and nullptr

Other simple solution could be to check if size is equal to 0 instead of checking if array is NULL.

The above change will solve your problem but your code can still be improved. Take into account the comments of other users. And make sure to free each dynamically allocated memory.

Joseph Larson · Answer 2 · 2022-03-07T22:21:09.903

Let's address a variety of things.

class DynamicArray {
public:
    int* array;
    int* size;
public:
    DynamicArray() {
        DynamicArray::size = new int;
        *DynamicArray::size = 0;
    }
};

A few things here. First, as others have suggested, there's zero reason to make size a pointer.

Next, it's a strong guideline / good idea to always initialize your fields when declared.

So this section of code can look like this:

class DynamicArray {
public:
    int* array = nullptr;
    int size = 0;
public:
    DynamicArray() {
    }
};

After that, please use nullptr instead of NULL. NULL is from C, but the correct value in C++ is nullptr.

Now, let's look at this bit of code.

void DynamicArray::addElementAtEnd() {
    cout << "\nPodaj liczbe calkowita: ";
    int* number = new int;
    cin >> *number;

    if (DynamicArray::array == NULL) {
        DynamicArray::array = new int[1];
        DynamicArray::array[0] = *number;
        delete number;
        (*DynamicArray::size)++;
        return;
    }
    int* buff = new int[*DynamicArray::size + 1];
    memcpy(buff, DynamicArray::array, (*DynamicArray::size) * sizeof(int));
    delete[] DynamicArray::array;
    buff[(*DynamicArray::size)] = *number;
    DynamicArray::array = buff;
    (*DynamicArray::size)++;
    delete number;
    return;
};

Aside from the extra colon on the end of the function (entirely not necessary), this is far more complicated than it needs to be. First, get rid of the int pointer. That's just ridiculous.

void DynamicArray::addElementAtEnd() {
    cout << "\nPodaj liczbe calkowita: ";
    int number = 0;
    cin >> number;

    int * newArray = new int[size + 1];
    newArray[size] = number;

    if (array != nullptr) {
        for (int index = 0; index < size; ++index) {
             newArray[index] = array[index];
        }
        delete [] array;
    }
    array = newArray;
    ++size;
}

A last comment -- it would make far more sense to pass in the new value as an argument to the method, and the calling test code should get the value you're adding. But you're just learning, so this works.

Note also that you shouldn't specify the class the way you have: DynamicArray::array. No one does that. Do it the way I did above.

You need to fix your constructor. After changing the type and initialization of the members, you didn't update the constructor to match. It should just be `DynamicArray() = default;` now. Also, your `addElementAtEnd()` is broken, as it doesn't preserve any existing values in the array. The OP's code did that much right, at least. But you lost that. — Remy Lebeau, Mar 07 '22 at 20:38
I didnt even know that `DynamicArray::array` thing was valid, to me it looks like static stuff. Glad you said to stop it — pm100, Mar 07 '22 at 22:02

Array is not resizing with operator new

2 Answers2