ArrayList Custom class

[MRKSalman0349]

I've made the ArrayList class like in java. This runs fine but when i stores values (i used for loop to store values) it always stores garbage value at index 2. Any solution please. And plz help me with suggestions that how can i make my code more efficient and good. Thank you....

  #include <iostream>
#include <cstdlib>
#include <ctime>
#include <windows.h>
#include <conio.h>


using namespace std;


class ArrayList{


public:

	ArrayList(){
		size = 0;	list = new int[0];

}

ArrayList(int d){
	size = 0;
	list = new int[d];
}


bool contains(int d){	bool find = false;	for( int i = 0; i < size; i++ ){

if(list[i] == d){	find = true;	break;	}

}	return find;	}


	ArrayList(int length , int *l){
		size = length;
		list = l;

}

//public int remove (int) : removes an element given at the specified index

int remove(int index){

int removed = -1;

if(checkIndex(index)){

removed = list[index];

int *l = new int[size - 1];

for( int i = 0; i < size ; i++ ){

if(i >= index){	l[i] = list[i+1];	}	else{

l[i] = list[i];	}	}

list = l;	size -= 1;

}	
else{

cout << "Index Out of bound\n";

}
}


//Checks if the arraylist is empty or not
bool isEmpty(){

if( size == 0 ){

return true;	}	else{

return false;	}	}


//clears the arryList	
void clear(){
delete list;

list = new int[0];	size = 0;	}


//public void add(int data) : adds the given element at the specified index

void add(int d){

//int *l = new int[size + 1];	for(int i = 0; i < size; i++){	l[i] = list[i];

//}

//
list[size] = d;
size++;	//for(int i = 0; i < size; i++){	list[i] = l[i];

}

//list = l;	size ++;	list[size - 1] = d;	}


//public int get(int index) : returns the element at the specified index

int get(int index){

if(checkIndex(index)){

return list[index];	}

else{

cout << "Index Out of bounds" << endl;


}
}

// pubkic int length() : returns the length of the ArrayList at current time :

int length() const{	return size;	}


//returns a new copy of the arraylist	
ArrayList clone(){

return ArrayList(length(), list);	
}

~ArrayList(){
	delete list;
	list = NULL;
}



private:

int size;	
int * list;


//private bool checkIndex(int index) : checks the index is not out of bounds :

bool checkIndex(int index){

bool right = false;	if(index >= 0 && index < size){

right = true;	}	else{	right = false;

}	return right;	}


};

int main(){
	ArrayList a;
	srand(time(0));
	for(int i = 0; i < 10; i++){
		int ad = rand() % 50;
		a.add(ad);
	}
	getch();
	for(int i = 0; i < a.length(); i++){
		cout << a.get(i) << " ";
	}
	cout << a.get(2);
	
}

[tipaye]

You're leaking memory in remove(). In line 58, free the memory:

delete [] list;
list = l;

Also in your destructor (and elsewhere) instead of

delete list;

, use delete [] list;

In add(), follow the example of delete, reallocate memory of the right size.

list[size] = d;

is out of bounds.

int *l = new int[size + 1];
for(int i = 0; i < size; ++i) l[i] = list[i];
l[size] = d;
delete [] list;
list = l;
size+=1;

In clone(), you need to copy the list array, otherwise you'll end up with 2 objects woning the same array, it will thus get destroyed twice.

ArrayList clone(){
int *l = new int[size]; // calling length here is inefficient
for(int i = 0; i < size; ++i) l[i] = list[i];
return ArrayList(length(), l);	
}

Generally speaking, allocating new memory and copying eveything each time you add or remove a single item is inefficient. You start with a reasonable sized buffer and track both the size of the buffer and the number of elements. Then you only need to allocate new memory when the buffer runs out. And when you're removing, you only need to move the items ahead of the element you're deleting back by one:

int remove(int index)
{
	int removed = -1;
	if(checkIndex(index))
	{
		removed = list[index];
		for( int i = index + 1; i < size ; i++ )
		{
			list[i] = list[i + 1];
		}
		
		size -= 1;
	}
	else
	{
		cout << "Index Out of bound\n";
		// you should throw an exception here, there's no valid return value
	}
	
	return removed;
}

Also don't forget to return values when you promise to do so remove(), get()).
I suggest a different indentation style as well, your code is very difficult to read, and mistakes are very likely, especially with multiple statements and conditionals on one line.
Finally, if you're not doing this just for practice, if this is a class you actually intend to use, then have a look at the standard library for much better alternatives. In any case have a look at the standard library for alternatives to raw pointers.

[tipaye]

P.S. "list" is a standard container, a name declared in namespace std - don't reuse the name like we've done above, or you'll run the risk of a name collision.

[coder777]

Why did you comment out line 90? That's the correct approach. You just need to set list = l;

Before you change list you should always delete[] it. Otherwise you will have a memory leak.

Note: When you use new[] you need to use delete[] not delete.

If size is 0 you can set list also to NULL.

If you want it more efficient you should introduce something like a capacity_size. Only when the capacity is exceeded you add certain amount of memory. This would avoid permanent allocation and copy.

[MRKSalman0349]

Thank you very much tipaye and coder777 .... Yours answers mean a lot to me ... You are realy great :) ... thank you so much......