C++: Am I managing memory the right way? [closed]

Question

Also, how would I go about putting my class in another file? Thanks!

#include <iostream>

using std::cout;
using std::endl;

class Array
{

int *arrayPointer;
public:

  Array(int size);
  ~Array(); 

  int get(int index);
  void set(int index, int value);

protected:
  int array[];
  int size;
};

Array::Array(int theSize){
  size = theSize;
  arrayPointer = new int[size]; 
}

Array::~Array(){
  delete[] arrayPointer;
}

int Array::get(int index){
  return *(arrayPointer + index);
}

void Array::set(int index, int value){
  if(index >= size || index < 0){
    cout << "Ooops, index out of bounds" << endl;
  }
  else{
    *(arrayPointer + index) = value;
  }
}

/* Let's see it in action now! */

int main(){
  int size = 3;
  Array array (size);
  array.set(0, 1);
  array.set(1, 2);
  array.set(2,3);
  cout << "Index 0 = " << array.get(0) << endl;
  cout << "Index 1 = " << array.get(1) << endl;
  cout << "Index 2 = " << array.get(2) << endl;
  array.set(4, 3);
  array.set(-1, 2);
}

Answer 1

First of all, you have no copy constructor (or assignment operator). This will be a problem in this case:

void foo(Array a) {
   //...
}

int main() {
   Array a(10);
   foo(a);
   //a is still around, but the pointer has been destroyed.
}

Since there will be two instances of Array using the same pointer. Once one of them goes out of scope, the destructor is called, so the delete[] happens. However, there is still another instance of Array with that same pointer, which has no way to know that delete happened. This is solved by following the "Rule of Three" See "Dynamically allocating an array of objects for a simple example.

In second place, you have a member called array. What's it for? It's never used so why have it?

In third place, some members are protected. Why? Does this mean you plan to have Array be inherited? In that case, you might want to make the destructor virtual. But I'd advise against inheritance, as it would feel counter-intuitive.

The implementation of get and set use unnecessary pointer arithmetic. Just do:

return arrayPointer[index];

This does the same thing and it's much cleaner to look at.

In fourth place, you do a check on the values of index for set, but not for get. Why not?

In fifth place, index is a signed integer. You may want to consider using size_t instead. It will also make the < 0 check redundant.

In sixth place, usage of get and set feels unnatural. You may want to look at operator overloading.

In seventh place, I would not use cout for the error message. Use assert(), cerr or an exception, depending on what your exact needs are.

In eighth place, you call it Array but you only allow integers to be stored. I'd either name it IntArray or make it a template to allow other types to be stored.

As for "Putting your class in another file", that might be a good idea, especially if you want Array to be used in multiple files. Put it in a header file.

Also, why create your own Array class? Is this for learning purposes? If not, you may want to use std::array or std::vector instead.

Answer 2

Two quick things:

---

Always make single variable constructors explicit (unless you actually want implicit construction, but you very rarely should).

 void f(Array a) { }
 f(5); //<-- runs fine.  basically does Array(5). 

---

Unless a class is specifically designed to handle output (a logging adapter, a formatter, etc), you shouldn't have output in a class.

What if this Array class were to be used in a GUI based application? That cout would be useless then. luiscubal suggested either assert or cerr, but I would argue that you should either throw an exception or give the method a bool return and return a flag of whether or not it was set. (There's no particularly pleasant way to recover from a failed assert, and cerr has the same problem of putting output in a class that has no business doing output.)