Pointers

[Tressor54321]

I got everything in this code running except for my remove function. What the project does is adds or removes an integer to a chain of integers created by the user. My add function works the first time but after that if I try to remove or add I believe it is pointing to the improper location and I don't know how to fix this.... Here is my code:

Header:
// adds "number" to the array pointed to by "arrayPtr" of "size". 
// Note the size of the array is thus increased. 
void addNumber(int *& arrayPtr, int number, int &size);

// removes a "number" from the "arrayPtr" of "size".
// if "number" is not there -- no action
// note, "size" changes
void removeNumber(int *& arrayPtr, int number, int &size);

// creates a copy of the array
// dynamically allocates an arry, makes "copy" point to it
// then copies contents of the "original" to "copy"
void copyArray(int *original, int *& copy, int size);

// prints the values in "arrayPtr" of "size" in sorted order
void output(int *arrayPtr, int size);

cpp file
#include <iostream>
#include "varArray.h"
using namespace std;

int main(void) {

	char operation;
	int newNum;
	int *arrayPtr = new int[0];
	int size = 0;

	do {

		cout << "Enter operation [a/r/q] and number: ";
		cin >> operation;
		if (operation != 'q') {

			cin >> newNum;

			if (operation == 'a')
				addNumber(arrayPtr, newNum, size);

			else if (operation == 'r')
				removeNumber(arrayPtr, newNum, size);
			output(arrayPtr, size);
			cout << endl;

		}

	} while (operation != 'q');

}

void addNumber(int *& arrayPtr, int number, int &size) {
	++size;
	int *newArr = new int[size];
	for (int i = 0; i < size-1; ++i)
		newArr[i] = arrayPtr[i];
	newArr[size-1] = number;
	delete [] arrayPtr;
	arrayPtr = newArr;

}

void removeNumber(int *& arrayPtr, int number, int &size) {

	bool found = false;
	for (int i = 0; i < size; ++i) {
		if (arrayPtr[i] == number)
			found = true;
	}

	if (found) {
		int *newArr = new int[size-1];
		for (int i = 0; i < size; ++i)
			if (arrayPtr[i] == number) continue;
		delete [] arrayPtr;
		arrayPtr = newArr;
		--size;
	}

}

void copyArray(int *original, int *& copy, int size) {
	for (int i = 0; i < size; ++i)
		copy[i] = original[i];

}

void output(int *arrayPtr, int size) {

	int *copy = new int[size];
	copyArray(arrayPtr, copy, size);
	cout << "Your numbers: ";
	while (size != 0) {            
		int smallest = copy[0];
		for (int i = 1; i < size; ++i) {
			if (copy[i] < smallest)
				smallest = copy[i];
		}
		cout << smallest << ' ';
		removeNumber(copy, smallest, size);
	}

}

[keskiverto]

Look at what std::vector does when you erase an element from it:
http://www.cplusplus.com/reference/vector/vector/erase/

Your remove does something entirely different.

[kbw]

In your removeNumber function, you need to be prepared to remove a number from the middle of the array, not just the end.

So you need to:
1. find the number to be removed. If the number isn't found, your done
2. allocate a new destination array that's one smaller than the incomming one
3. copy the numbers upto one before the entry to be removed
4. copy the numbers above it, down.
5. fixup size, release the original array and return the new destination array

As you're allocating the new array first, you're loosing the last element. Also, you're not doing step 3.

It's clear that your program uses two items that go together, arrayPtr and size. When start using structs, you'll see that it's possible to bind these two together. And when you start using classes, you can create a new type based on this relationship.

[Tressor54321]

So to do that, right after I am making the size one smaller, I would use my copyarray function and all would be well?

[kbw]

I would use my copyarray function and all would be well?

Well, you're not copying the entire array, just two sections.

So you could use that function, but you'd need to modify that function to copy a range.

[Tressor54321]

Alright now I'm completely lost.... lol

[kbw]

You will never need to copy the entire array during your remove, so forget copyarray.

You need to copy upto the element to be removed, then from the next element to the end.

It's not clear what's confusing.

[Tressor54321]

So I need to edit my copyarray? I mean I get what you're saying I'm just lost on how I would go about doing it, I guess.

[keskiverto]

Your copyArray() is fine; it does copy a range. You just have to call it twice, with different parameters.

However, is the meaning of removeNumber() to remove the first occurrence, or all occurrences of value 'number'?


Note: std::vector has two properties: allocated size and used size. It does not allocate-copy-deallocate on every erase(). It just adjusts the 'used size' and copies the suffix.

[Tressor54321]

The purpose of remove is just to remove the first instance.

[Tressor54321]

If I call copyArray twice in the output function where would I put it and what would I be using for the parameters?

[Tressor54321]

I replaced my copyArray and removenumber functions with this

void copyArray(int *original, int *& copy, int size) {
	copy = new int[size];

	for (int i = 0; i < size; ++i)
		copy[i] = original[i];

}
void removeNumber(int *& arrayPtr, int number, int &size) {

	bool found = false;
	for (int i = 0; i < size; ++i) {
		if (arrayPtr[i] == number)
			found = true;
	}

	if (found) {
		int *newArr = new int[size-1];
		copyArray(arrayPtr, newArr, size);
		for (int i = 0; i < size; ++i)
			if (arrayPtr[i] == number) continue;
		delete [] arrayPtr;
		arrayPtr = newArr;
		--size;
	}

}

and what I am now getting fixed the values issue although it does not add correct values. It continuously adds the smallest value to the collection

[keskiverto]

You have changed copyArray and removeNumber and say that addition malfunctions? Addition does not use those two functions.


IMHO the new copyArray is worse than the old. The old (although it does not do what its comment promises) could do this:

int a[] = { 5, 4, 3, 2, 1 };

int b[4];
copyArray( a, b, 1 );
copyArray( a+2, b+1, 3 );
// assert: b == { 5, 3, 2, 1 } 

The new cannot (but it matches the description).

The removeNumber() has severe logic issues.

1. Knowing that the array contains number is not enough. You want to know where it is. The first loop should achieve that.

2. Line 18 copies 'size' elements into newArr, which appears to have only 'size-1' elements. However, the new copyArray does discard (i.e. leak) the memory allocated on line 17 and copies to different 'size'-long array.

3. Loop on lines 19-20 does absolutely nothing.

4. At end the 'size' does no longer match to the size of the 'arrayPtr' array.