Array indexing with copy constructor, assignment operator and index operator overload

Question

The code below is a custom array class that handles indexing, copying, printing, etc. explicitly.
Is there a better approach for specification (declaration and definition) of the assignment operator (with swap function) and the index [] operator?
Furthermore, are there any improvements that can be made to the other methods to improve functionality?

// Overloading operators for Array class
#include<iostream>
#include<cstdlib>
using namespace std;
// A class to represent an integer array
class Array{
private:
 int *ptr;
 int size;
public:
 Array(int *p = NULL, int s = 0);
 Array(const Array&);
 ~Array();
 Array& operator= (Array);
 // Overloading [] operator to access elements in array style
 int &operator[] (int);
 // Utility function to print contents
 void print() const;
 friend void swap(Array& first, Array& second);};
// Implementation of [] operator. This function must return a
// reference as array element can be put on left side
int &Array::operator[](int index){ 
 if (index >= size || index < 0){
 throw out_of_range("Index out of Range error");
 }
 return ptr[index];
}
// constructor for array class
Array::Array(int *p, int s){
 size = s;
 ptr = NULL;
 if (s != 0){
 ptr = new int[s];
 for (int i = 0; i < s; i++)
 ptr[i] = p[i];}
}
// destructor for array class
Array::~Array(){
 delete[] ptr;
 ptr = NULL;}
// copy constructor for array class
Array::Array(const Array& A) { 
 size = A.size;
 ptr = new int[size];
 for (int i = 0; i < size; i++)
 ptr[i] = A.ptr[i];}
//swap friend function of assignment operator
void swap(Array& first, Array& second){
 using std::swap;
 swap(first.size, second.size);
 swap(first.ptr, second.ptr);}
//Assignment operator for array class
Array& Array::operator=(Array other){
 swap(*this, other); 
 return *this;}
//print function for array elements
void Array::print() const{
 cout << "{";
 for(int i = 0; i < size; i++)
 cout<<ptr[i]<<" ";
 cout<<"}"<<endl;}
// Driver program to test above methods
int main()
{
 int a[] = {1, 2, 3, 4, 5, 6};
 Array arr1(a, 6);
 arr1[0] = 7;
 arr1.print();
 Array arr2 = arr1;
 arr2.print();
 arr1[-1] = 4;
 return 0;
} 

• Toby Speight: Welcome to Code Review! Your edit invalidated an answer, which breaks the question-and-answer nature of this site. See What should I do when someone answers my question?.

Answer 1

This isn't really a code review so much as help on how to get this to compile.

• Array::Array(int *p = NULL, int s = 0){ default parameters go on the declaration, not the definition.*
• Array::Array& operator=(Array other){ Qualify the operator with Array::, not the return type. You're not returning an Array::Array**, you're defining the operator= member function of Array
• void Array::swap(Array& first, Array& second){ swap is not a member of array, it's a friend. Remove the Array::

*This is because the client (caller) to the method needs to know what the defaults are supposed to be, since default parameters are mostly a syntactic shortcut. If you put the defaults into the definition (e.g. a cpp/.o file), a client which includes the header cannot see the definition and thus cannot use them. In other words, there exists no Array::Array() constructor right now, there's only the Array::Array(int *, int), but through C++ magic it can look as though Array::Array() is being called.

** An Array::Array type is impossible because you can't name a child type the same as its parent type. It would be ambiguous with the constructor.

• Darnoc Eloc: Thanks for the feedback! Is it possible to avoid the Aborted (core dumped) message when the out_of_range error is thrown?
• Darnoc Eloc: Furthermore, how would one implement a try-catch statement for invalid index values?
• butt: I would argue that you should not throw an exception on [], but that's debatable. My reasoning is that [] is canonically a dangerous operation in C++, and should be semantically thought of as a dereference of a potentially out-of-bounds address, usable only if you're absolutely certain it will not fail. This type of dangerous operation is integral to C++ since there's no convenient layer below it to handle it better. A method call like GetAt(IndexType) or something might throw by testing the bounds -- not by catching another exception, the class knows whether the argument is valid or not.

Answer 2

Don't using namespace std; - especially not in a header, where it inflicts the harm on every source that includes the header.

Prefer nullptr to NULL, because the former is more strongly typed.

Use std::size_t for indexing, rather than int.

When overloading operator[], it's usually necessary to provide two versions:

int& operator[](std::size_t);
int const& operator[](std::size_t) const;

Usually, operator[] doesn't throw. Provide a separate "checked" interface if your clients need that as well. That's normally named at().