1
\$\begingroup\$

I am a mathematician trying to learn how to become a better programmer in C++. I am currently now trying to write the main data structures from scratch. I have now completed a generic array class and I wanted to see if I was missing anything or if there is anything I can improve on.

Here is the header file:

#ifndef Array_h
#define Array_h

#include <iostream>

template <class T>
class Array {
private:
    int maxSize; // capacity of array
    int length;
    T *array;

public:
    Array(int size = 100);
    ~Array();
    void print() const;
    bool isEmpty() const;
    bool isFull() const;
    int maxListSize() const;
    int listSize() const;
    T front() const;
    T back() const;
    void swap(T& x, T& y);
    void sort();
    int search(const T& item);
    void insert(int location, const T& item);
    void insertAtTop(const T& item);
    void deleteItem(const T& item);

    void operator=(const T& temp);
    T& operator[](int index) { return array[index];}
};

template <class T>
Array<T>::Array(int size) {
    if (size <= 0) {
        std::cout << "Size cannot be less than 0";

        maxSize = 100;
        array = new T[maxSize];
    }
    else {
        maxSize = size;
        length = 0;
        array = new T[maxSize];
    }

}

template <class T>
Array<T>::~Array() {
    delete[] array;
}

template <class T>
void Array<T>::operator=(const T &temp) {
    for(int i = 0; i < length; i++) {
        array[i] = temp;
    }
}

template <class T>
void Array<T>::print() const {
    for(int i = 0; i < length; i++) {
        std::cout << array[i] << "\t";
    }
}

template <class T>
bool Array<T>::isEmpty() const {
    return (length == 0);
}

template <class T>
bool Array<T>::isFull() const {
    return (length == maxSize);
}

template <class T>
int Array<T>::maxListSize() const {
    return maxSize;
}

template <class T>
int Array<T>::listSize() const {
    return length;
}

template <class T>
T Array<T>::front() const {
    return array[0];
}

template <class T>
T Array<T>::back() const {
    return array[length-1];
}

template <class T>
void Array<T>::swap(T& x, T& y) {
    T temp = x;
    x = y;
    y = temp;
}

template <class T>
void Array<T>::sort() {
    for(int i = 0; i < length - 1; i++) {
        for(int j = 1; j < length - i - 1; j++) {
            if(array[j] > array[j-1]) {
                std::swap(array[j], array[j-1]);
            }
        }
    }
}

template <class T>
int Array<T>::search(const T& item) {
    for(int i = 0; i < length; i++) {
        if(array[i] == item){
            return i;
        }
        else {
            return 0;
        }
    }
}

template <class T>
void Array<T>::insert(int location, const T& item) {
    if(length != maxSize) {
        int i = length;

        for(;i > location; i--) {
            array[i] = array[i-1];
        }
        array[i] = item;
        length++;
    }

}

template <class T>
void Array<T>::insertAtTop(const T& item) {
    if(length != maxSize) {
        array[length] = item;
        length++;
    }
}

template <class T>
void Array<T>::deleteItem(const T& item) {
    bool flag = false;
    int location = 0;
    for(;location < length; location++) {
        if(array[location] == item) {
            flag = true;
            break;
        }
    }

    if(flag) {
        for(int i = location; i < length - 1; i++) {
            array[i] = array[i+1];
        }
        length--;
    }
    else {
        std::cout << "Element not found" << std::endl;
    }
}




#endif /* Array_h */

Here is the main file that tests some of the member functions (I have checked all and they work).

#include <iostream>
#include "Array.h"

int main(int argc, const char * argv[]) {

    Array<int> arr(5);
    arr.insertAtTop(2);
    arr.insertAtTop(4);
    arr.insertAtTop(6);
    arr.insertAtTop(8);
    std::cout<<"\n------------------------------------------------------------\n";
    std::cout<<"---------------Displaying All contents of array---------------";
    std::cout<<"\n------------------------------------------------------------\n";
    arr.print();

    std::cout<<"\n--------------------------------------------------\n";
    std::cout<<"-------------Inserting At Particular--------------";
    std::cout<<"\n--------------------------------------------------\n";
    arr.insert(1, 12);
    arr.print();

    std::cout<<"\n--------------------------------------------------\n";
    std::cout<<"----------------Deleting an Item-----------------";
    std::cout<<"\n--------------------------------------------------\n";
    arr.deleteItem(12);
    arr.print();

    return 0;
}
\$\endgroup\$
3
\$\begingroup\$

Implementing from scratch to learn about it is a worthy approach and I see that as the subject of presentations at the major conferences.

You should use the naming convention from the standard containers. You wrote a vector, not an array. You don’t insertAtTop, you push_back, etc.


T *array;

The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.

⧺C.149 — no naked new or delete.

You should probably make this a unique_ptr as a drop-in replacement without otherwise changing the architecture.


Array(int size = 100);

Make that explicit. Do you know why?


void print() const;

No, don’t make that a member function. Provide a general way for users to access the elements. Then a printing function can be written using the public API.


void swap(T& x, T& y);

Huh? That is a (non-static) member, so why is it taking two additional parameters?

Skipping down to look at the implementation: you wrote it as a non-member. So it should be a free (template) function swap.

As for how/whether it should be implemented, I’ll get back to that.


void sort();

No, just let the user do std::sort with your container or portions thereof.

int search(const T& item);

Again, can your implementation do better than a linear search? Just let the user use std::find and std::find_if on your collection!


void operator=(const T& temp);

It is not normal to have a void return here. You should return *this;.

Edit: OK, that’s not a copy assignment operator at all. void is fine. But the automatically-generated copy assignment code will not work right, as described next for the copy constructor.


Where is your copy constructor? Your class will seriously malfunction if you write something like:

Array<int> arr(5);
  ⋮
Array<int> a2 = arr;

See The rule of three/five/zero.


You should provide move semantics for efficient usage of the array object, and for faster insert/delete etc. when T is a complex type.


template <class T>
Array<T>::Array(int size) {
    if (size <= 0) {
        std::cout << "Size cannot be less than 0";

        maxSize = 100;
        array = new T[maxSize];
    }
    else {
        maxSize = size;
        length = 0;
        array = new T[maxSize];
    }

}

Don’t complain and use a different value. Treat it as an error.

if (size <= 0)  throw std::invalid_argument ("Array size negative");

You are actually constructing the full number of elements of type T. So your length is what? How many of them you care about? Normally, a collection will not construct elements that are not used.


template <class T>
Array<T>::~Array() {
    delete[] array;
}

If you used a unique_ptr like I suggested above, you would not need to write a destructor at all! (And, it would not automatically generate a bad copy constructor!)


template <class T>
void Array<T>::operator=(const T &temp) {
    for(int i = 0; i < length; i++) {
        array[i] = temp;
    }
}

Hmm, that’s not an assignment operator at all. It assigns a common value to all elements.

Just use std::fill_n.


The various trivial implementations should be in-line in the class definition.


template <class T>
void Array<T>::sort() {
    for(int i = 0; i < length - 1; i++) {
        for(int j = 1; j < length - i - 1; j++) {
            if(array[j] > array[j-1]) {
                std::swap(array[j], array[j-1]);
            }
        }
    }
}

A bubble sort? Are you kidding?!


Your front and back functions don’t check for zero length. You have a deleteItem function, so the user could delete all of them!

Speaking of deleteItem, you seem to duplicate the find code first. Just use find!


insert functions silently do nothing if the container is full. They should throw an exception.

Copying all the elements down one spot in a loop — can you find a std algorithm for that instead?

\$\endgroup\$
  • \$\begingroup\$ I am going to address each point you made and make the changes. After doing so should I make a new post? \$\endgroup\$ – Snorrlaxxx May 27 '18 at 18:37
  • \$\begingroup\$ Yes, feel free to post again with the new version. You ought to link to this previous version when introducing the code. \$\endgroup\$ – JDługosz May 28 '18 at 21:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.