current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

This is a simple implementation of vector in C++. Please offer your reviews to improve the code.

#ifndef VECTOR_HPP
#define VECTOR_HPP

template <typename T> class vector{
    int size;//size allocated on the memory
    int index;//current number of elements
    T* arr;
    void resize(){
        T* temp = new T[size];
        for (int i = 0; i <index ; ++i) {
            temp[i] = arr[i];
        }
        delete[] arr;

        T* arr = new T[size*2];
        for (int i = 0; i < index ; ++i) {
           arr[i] = temp[i];
        }
        size = size*2;
        delete[] temp;

    }
public:
    vector(){
        size = 50;
        index = 0;
        T* arr = new T[size];
    }
    vector(int sz){
        size = sz;
        index = 0;
        T* arr = new T[size];

    }
    ~vector(){
        delete[] arr;
    }

    typedef T* iterator;

    iterator begin(){
        return arr;
    }

    iterator end(){
        return arr + index;
    }

    T* front(){
        return arr[0];
    }

    T* back(){
        return arr[index];
    }
    void push_back(const T& t){
        if((size - index) == 1){
            resize();

        }
        arr[index+1] = t;
        index++;

    }
    void pop_back(){
        index--;
    }

    void insert(iterator pos, const T& t){
        if((size -index) < 1) resize();
        int npos = pos - arr;
        for (int i = index; i > npos ; i--) {
            arr[i+1] = arr[i];
        }
        arr[npos] = t;
        index++;

    }

    void erase(iterator pos){
        int npos = arr - pos;
        if((size - index) < 1) resize();
        for (int i = npos; i <index-1 ; ++i) {
            arr[i] = arr[i+1];
        }
        index--;

    }

    T& operator[](int){

    }

    int length(){
        return index;
    }
    bool empty(){
        return index == 0;
    }


};

#endif //VECTOR_HPP
share|improve this question
    
This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. – SuperBiasedMan Oct 20 '15 at 14:22
1  
There are also some broken parts (does index mean the index of the last element of the number of elements in the array, the code can't seem to decide) – ratchet freak Oct 20 '15 at 14:23
up vote 7 down vote

There are three severe errors in your code:

Rule of Three/Five/Zero

This is an invaluable resource. Anytime you write a user-defined destructor, you will need to write the copy constructor and assignment. You lack the copy constructor or assignment, so the compiler will happily provide one for you, and that one will not do the right thing:

vector<int> v;        // we have an array of size zero with capacity 50
vector<int> v2 = v;   // we do not have two arrays: both vectors have
                      // the SAME pointer
// <== v2 gets destroyed, we run delete [] array
// <== v gets destroyed, we run delete [] array again! boom

In addition to the double free, you get the other issue in that the user might expect a deep copy but actually v2 is basically just a Java-style reference to v. Fixing this is an absolute must.

Resizing

This is definitely non-functional. You delete your member arr, then, separately from that, you create a locally scoped array arr that you write stuff into. At the end of resize(), the vector object will have a deleted local array!

The correct way to resize() is to allocate a new array (only have to do one!) of double the size, to copy/move from your member into the new array, and then simply change your member to point to the new array. This should be one allocation (of the doubled array) and one deletion (of the old array).

Off-by-one

The first push_back() writes into arr[1]. What goes into arr[0]?

Once you fix those, there are other issues with your code:

Limits on Types

When you construct like this:

vector(){
    size = 50;
    index = 0;
    T* arr = new T[size];
}

You are adding a restriction on T. Namely, that it must be default constructible:

struct X { X(int ) { } };
vector<X> v;

leads to:

main.cpp: In instantiation of 'vector<T>::vector() [with T = X]':
main.cpp:108:15:   required from here
main.cpp:26:28: error: no matching function for call to 'X::X()'
         T* arr = new T[size];
                            ^

The way to avoid this is to use global ::operator new to allocate memory without constructing any objects.

Naming

You have member variables named size, which is actually the capacity of the vector, and index, which is its size. Those names are confusing and should be changed to reflect their actual purpose.

Object Destruction

A vector owns its objects. When you pop or erase from a vector, it must release ownership - which will involve explicitly calling the destructor on your type. You do not currently do this.

Resizing, part 2

Resizing should only happen on insertion. You also do it on erase(). This is illogical. Why would removing an element lead to doubling in the array size?

operator[]

Where's the body?

const overloads

If I have a const vector<int>, I cannot do anything with it. That's not useful. empty() and length() (which should be spelled size()) can just be const member functions. There also need to be const overloads for begin(), end(), operator[], front(), and back().

share|improve this answer
    
Good review, and now I also voted. – Deduplicator Oct 21 '15 at 14:30
    
I'm a little new to codereview, do you want me to update my code with your suggestions. Is that how it works. – Antithesis Oct 21 '15 at 15:37
    
@Antithesis No, never change code in a question (unless it was originally mis-copied, etc). If you want to update your code, you can create a followup question. – Barry Oct 21 '15 at 16:11
    
@Barry thanks, that's what I wondered. – Antithesis Oct 21 '15 at 17:58
up vote 0 down vote

If you take a look at the standard containers, they have both a capacity and a size.
A container has reserved space for capacity elements, but only contains size ones. The rest of the space is unused, no object is constructed there.

In contrast, your container constructs elements on allocating space.

I'm blaming Barry for only leaving so little else to say.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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