patterncppModerate
Simple array implementation without bounds-checking
Viewed 0 times
withoutsimplearraycheckingimplementationbounds
Problem
I am preparing myself for some job interviews. This is a simple array (with the error handling left out for brevity). Any input or suggestions on style and the use of templates would be appreciated.
#include
template
class Array {
public:
explicit Array(const int&);
Array(const int&, const T&);
Array(const Array&);
virtual ~Array();
T getVal(const int&) const;
void setVal(const int&, const T&);
T& operator[] (T);
Array& operator= (const Array&);
template
friend std::ostream& operator&);
private:
T* arr;
int size;
};
template
Array::Array(const int& init_size) {
arr = new T[init_size];
size = init_size;
}
template
Array::Array(const int& init_size, const T& init_value) {
arr = new T[init_size];
size = init_size;
for (int i = 0; i
Array::Array(const Array& original) {
size = original.size;
arr = new T[size];
for (int i = 0; i
Array::~Array() {
delete [] arr;
}
template
T Array::getVal(const int& index) const {
return arr[index];
}
template
void Array::setVal(const int& index, const T& value) {
arr[index] = value;
}
template
T& Array::operator[] (T index) {
return arr[index];
}
template
Array& Array::operator= (const Array& copy) {
if (arr == copy.arr)
return *this;
size = copy.size;
delete [] arr;
arr = new T[size];
for (int i = 0; i
std::ostream& operator& self) {
out << "[ ";
for (int i = 0; i < self.size-1; ++i) {
out << self.arr[i] << ", ";
}
out << self.arr[self.size-1] << " ]";
return out;
}Solution
In the constructor that just takes a size. I would initialize all members to their default value even if POD.
Use the initializer lists in your constructors.
Use copy and swap idiom for assignment operator (it automatically provides the strong exception guarantee).
If you want to do this the hard way then it should look like this:
Alternatively use the copy and swap idiom
Note this can be optimized too:
You don't need a get/set methods. Infact get/set is an obvious indication of bad design.
Just remove both these methods. What you want to do is return a reference to the internal object in your overload of operator[] (which you do) bit the index is an integer usually.
You also need to provide a verision to use when your array is const.
If you provide an output operator you should generally by symmetric and provide an input operator.
template
Array::Array(const int& init_size)
{
arr = new T[init_size]();
// ^^^^^ Add braces. on types it makes no difference.
// On POD it will force zero initialization.
size = init_size;
}Use the initializer lists in your constructors.
template
Array::Array(const int& init_size)
: arr(new T[init_size]())
, size(init_size)
{}Use copy and swap idiom for assignment operator (it automatically provides the strong exception guarantee).
template
Array& Array::operator= (const Array& copy)
{
if (arr == copy.arr) // This is the wrong test.
return *this; // Though it works as a side effect you will confuse the maintainer.
size = copy.size;
delete [] arr; // breaks the strong exception gurantee.
// What it the next line fails? Then you are left with an object
// in an inconsistent state. (arr points at de-allocated memory)
arr = new T[size];
for (int i = 0; i < size; ++i)
{
arr[i] = copy.arr[i];
}
return *this;
}If you want to do this the hard way then it should look like this:
template
Array& Array::operator= (const Array& copy)
{
if (this == ©) // Return quickly on assignment to self.
{ return *this;
}
// Do all operations that can generate an expception first.
// BUT DO NOT MODIFY THE OBJECT at this stage.
T* tmp = new T[size];
for (int i = 0; i < size; ++i)
{
tmp[i] = copy.arr[i];
}
// Now that you have finished all the dangerous work.
// Do the operations that change the object.
std::swap(tmp, arr);
size = copy.size;
// Finally tidy up
delete tmp; // Notice the swap above.
// Now you can return
return *this;
}Alternatively use the copy and swap idiom
template
Array& Array::operator=(Array const& rhs)
{
// 1 Copy
Array copy(rhs); // Use the copy constructor to make a safe copy.
// 2 Swap
swap(arr, copy.arr);
swap(size, copy.size);
} // 3 as the local copy goes out of scope it destroys the old array.Note this can be optimized too:
Array& Array::operator=(Array rhs) // 1 implicit copy as parameter is passed by value
{
// 2 Swap
swap(arr, copy.arr);
swap(size, copy.size);
} // 3 as the local copy goes out of scope it destroys the old array.You don't need a get/set methods. Infact get/set is an obvious indication of bad design.
template
T Array::getVal(const int& index) const {
return arr[index];
}
template
void Array::setVal(const int& index, const T& value) {
arr[index] = value;
}Just remove both these methods. What you want to do is return a reference to the internal object in your overload of operator[] (which you do) bit the index is an integer usually.
template
T& Array::operator[] (size_t index)
{ // ^^^^^^^ Changed type here.
return arr[index];
}You also need to provide a verision to use when your array is const.
template
T const& Array::operator[] const (size_t index)
//^^^^^ Here ^^^^^ and here.
{
return arr[index];
}If you provide an output operator you should generally by symmetric and provide an input operator.
Code Snippets
template <class T>
Array<T>::Array(const int& init_size)
{
arr = new T[init_size]();
// ^^^^^ Add braces. on types it makes no difference.
// On POD it will force zero initialization.
size = init_size;
}template <class T>
Array<T>::Array(const int& init_size)
: arr(new T[init_size]())
, size(init_size)
{}template <class T>
Array<T>& Array<T>::operator= (const Array& copy)
{
if (arr == copy.arr) // This is the wrong test.
return *this; // Though it works as a side effect you will confuse the maintainer.
size = copy.size;
delete [] arr; // breaks the strong exception gurantee.
// What it the next line fails? Then you are left with an object
// in an inconsistent state. (arr points at de-allocated memory)
arr = new T[size];
for (int i = 0; i < size; ++i)
{
arr[i] = copy.arr[i];
}
return *this;
}template <class T>
Array<T>& Array<T>::operator= (const Array& copy)
{
if (this == ©) // Return quickly on assignment to self.
{ return *this;
}
// Do all operations that can generate an expception first.
// BUT DO NOT MODIFY THE OBJECT at this stage.
T* tmp = new T[size];
for (int i = 0; i < size; ++i)
{
tmp[i] = copy.arr[i];
}
// Now that you have finished all the dangerous work.
// Do the operations that change the object.
std::swap(tmp, arr);
size = copy.size;
// Finally tidy up
delete tmp; // Notice the swap above.
// Now you can return
return *this;
}template <class T>
Array<T>& Array<T>::operator=(Array const& rhs)
{
// 1 Copy
Array copy(rhs); // Use the copy constructor to make a safe copy.
// 2 Swap
swap(arr, copy.arr);
swap(size, copy.size);
} // 3 as the local copy goes out of scope it destroys the old array.Context
StackExchange Code Review Q#3944, answer score: 10
Revisions (0)
No revisions yet.