My own std::vector

I've made my own std::vector but it's the first time that I work with template and new/delete. The code works, but surely there are a lot of things that are wrong. Can you read the code and say me if I have coded it the right way?

(main is a test.)

```
#ifndef __STDVECTOR__
#define __STDVECTOR__

#include

using namespace std;

template
class StdVector{
private:
T *buffer;
unsigned int capacity;
public:
//Constructor.
StdVector(){
capacity=0;
buffer=new T[capacity];
}
//Copy constructor.
StdVector(const StdVector &asv){
int i;

capacity=asv.getCapacity();
buffer=new T[asv.getCapacity()];
for (i=0; i=capacity){
//Out of range.
throw exception();
}
else{
return buffer[index];
}
}
StdVector &operator=(const StdVector &obj){
capacity=obj.getCapacity();
delete []buffer;
buffer=new T[capacity];
buffer=obj.getBuffer();
return *this;
}
unsigned int getCapacity() const{
return capacity;
};
};

#endif

int main(){
try{
StdVector test;
StdVector test2;
unsigned int i;

test.push_back(5);
test.push_back(4);
test.push_back(3);
test.push_back(2);
test.push_back(1);
test.push_back(0);
test.push_back(-1);
test.push_back(-2);
test.push_back(-3);
test.push_back(-4);
test.push_back(-5);
for (i=0; i<test.getCapacity(); i++){
cout << test[i] << endl;
}
test2.push_back("Hello");
test2.push_back(" ");
test2.push_back("World");
test2.push_back(".");

Don't use double underscore.

#ifndef __STDVECTOR__
#define __STDVECTOR__

Identifiers with double underscores are reserved for the implementation.

see: What are the rules about using an underscore in a C++ identifier?

Stop using

using namespace std;

This kind of thing can break so much code. Putting it in your header file will get you banned from open source projects as you corrupt the global namespace of any compilation unit that includes your header. Even in your own source files it is a bad idea as it potentially introduces hard to spot errors.

see: Why is “using namespace std” in C++ considered bad practice?

Don't build objects that have not been added.

Your current design is flawed.

template 
    class StdVector{
            T *buffer;
            ...
                buffer=new T[capacity];

Because you only have one size value (a capacity) your code becomes tremendously inefficient (as we see when we get to push back).

You should have two sizes.

• The capacity: The amount of space allocated for objects in your vector.

• The size: The number of objects in the vector.

Without both these sizes they have to be the same value. This means you can not pre-allocate space and each time you add or remove elements you must resize the vector and this includes both allocating new space and copying all the elements into the newly allocated space

I go into a lot of detail in my article:

Vector - Resource Management Allocation

Prefer to use initializer list.

StdVector(){
            capacity=0;
            buffer=new T[capacity];
        }

This is totally fine and works. But it is a bad habit. If you change the types of the members you are potentially making your class inefficient as the members are constructed before the body of the class is entered. You then modify them in the body.

StdVector()
            : buffer(new T[capacity])
            , capacity(0)
        {}

Rule of three

Your assignment operator is way down in your code. I initially thought you were violating the rule of three. Put your assignment operator close to the constructors.

Assignment operator not exception safe.

This assignment operator is the classic first attempt.

StdVector &operator=(const StdVector &obj){
                capacity=obj.getCapacity();
                delete []buffer;
                buffer=new T[capacity];
                buffer=obj.getBuffer();
                return *this;
            }

But it is prone to leaking and leaving the object in an inconsistent state if there are exceptions (in the constructor of T).

You should use the copy and swap idiom.

StdVector &operator=(StdVector tmp) // notice the pass by value
            {                                   // this creates a copy.
                tmp.swap(*this);
                return *this;
            }
            void swap(StdVector& other) noexcept
            {
                using std::swap;
                swap(capacity,   other.capacity);
                swap(buffer,     other.buffer);
            }

I cover this in a lot of detail in:

Vector - Resource Management Copy Swap

Push back ineffecient

You are making a copy of the whole vector each time you add an element. But you are doing it twice to make even more inefficient that the original inefficiency imposed by your design.

void push_back(T obj){
                StdVector oldSV(*this);
                int i;

                capacity++;
                delete []buffer;
                buffer=new T[capacity];
                for (i=0; i<oldSV.getCapacity(); i++){
                    buffer[i]=oldSV[i];
                }
                buffer[i]=obj;
            };

You can get rid of one copy like this:

void push_back(T obj){    
                int newCapacity = capacity + 1;
                T*  newBuffer   = new T[newCapacity];

                for (int i = 0; i < capacity; ++i){
                    newBuffer[i]=capacity[i];
                }
                swap(capacity, newCapacity);
                swap(buffer,   newBuffer);
                delete [] newBuffer;
            };

Still not very good. But better than the original.

For Loops incrementing iterators.

• Prefer to declare the loop variable inline.

• Prefer to use prefix increment (not all iterators are as efficient as int).

Like this:

for (int i = 0; i < capacity; ++i){
                    newBuffer[i]=capacity[i];
                }

Badly named method.

This does not get the buffer.

T getBuffer() const{
                if (capacity==0){
                    throw exception();
                }
                return *buffer;
            };

It returns a copy of the first element in the vector.

Efficiency.

Normally in C++ the operator[] does unchecked access to the elements. Because there is no need to pay for the check in the method if the calling code already does the check.

```
T &operator[](int index) const{

StackExchange Code Review Q#138389, answer score: 7