Creating a custom Vector class

Questions:

How would it differ if I set elem & sz in the body of the constructor? As of now, they are being declared after the method declaration, but before the start of the actual function (in the Copy Constructor).

It's best to set everything in the initializer list (good habit when things could be arbitrary objects).

removed

I use the sz variable, as an upperbound for a loop. What is safest? Should I use sz, this->sz, size() or this->size()? size() is a function within the code which returns this->sz;

Just use sz.
Use of this-> is discouraged as it means you are trying to force the compiler to resolve a particular variable that is shadowed which means you are using a bad naming scheme that is suitable to shadowing.

Shadowing causes all sorts of problems. One way to get around it is to force the use of this-> on all members (which is fine until you accidentally miss one).

The better option is turn up your compiler warnings so it warns you about shadowing. Then treat all warnings as errors (your code should compile warning free on the highest warning level).

Am I overusing const? Since there are no assignment operators within the function, it doesn't perform any changes to its class members - so is the last const useless?

1. Don't bother with const on the return type when returning by value.
2. Be judicious on your use of returning Vector by const ref.
   A lot of the time you want to return *this as a ref to allow chaining.

My operator==(Vector&) function is pretty ugly. Any suggestions for a nicer/more efficient solution?

You could use a standard algorithm to help you check. But your code does not look that bad.

In my operator--() function (which is meant to remove the last element in the vector), I simply reduce the sz variable for the calling vector by 1. I'm not actually deleting anything. Is this bad practice? What is a better solution? Is there a way to delete a single entry in an array?

Normally vector contain two sizes.

1. The number of elements currently in the vector.
2. The amount of space allocated.
   This is space allocated but currently unused. Normally when creating arrays you allocate slightly more space than you need. So you can use it without having to reallocate the whole data segment and copy it just for adding a single value (or when deleting a value you just reduce the size and can safely re-use it).

I understand that a (const Vector& a argument to the operator+(..) function) is a const, but the function doesn't change the argument whatsoever. If the function remains as is, could I remove the const declaration which prepends the argument?

?

In my pushBack(int) function, an int array (called temp) is created using new - which means it must be deleted. However, using _CrtDumpMemoryLeaks();, I get no objection from the compiler. Is this because it automatically self-deconstructs because it's an int array?

?

Why shouldn't my size() return sz instead of this->sz. Am I correct in understanding this is primarily for multithreaded reasons?

Its the same thing. See my description of this usage above.

Comments on Code:

Always prefer to use the initializer list. The compiler is going to plant the appropriate code anyway. May as well take advantage of this fact and use the compiler to put the corret initial values in place. (Note with POD data there is no initialization but for user defined types there will be. So it will construct the object members before the function is entered.

Vector::Vector(int s) //Constructor w/ size
// Add initializer list.

The following can be done in a single line:

    elem = new int[s];
    for (int i = 0; i < s; i++)
        elem[i] = 0; //init elems to 0

    //
    elem = new int[s](); // zero initialize all members.
                         // Or default construct them if you change the Vector to
                         // use generic types.

Note: members are initialized in the order they are declared in the class declaration (not the order they appear in the initializer list). If you turn up wanings the compiler will warn you about this. If you make the compiler treat warnings as errors (as you should be doing) then it will not compile if the initializer list is in the wrong order.

Vector::Vector(const Vector& a)
    :elem{ new int[sz] },             // Is `sz` defined at this point ???
                                      // I can't tell because I don't have the class
                                      // declaration.

    sz{ a.sz } ////QUESTION 1         // But the order here is not conjusive to read
                                      // as it looks like you are setting sz after
                                      // you have used it in the previous line.

In this one you don't initialize elem.

Vector::Vector() //empty constructor - functionally useless
{
    sz = 0;
}

This means it is pointing at some random piece of memory. When the destructor is run you delete a random unitialized pointer.

You actually did the assignment operator correctly and the hard way. Though if you had user defined types rather than int in your object it may not have worked correctly.

Vector& Vector::operator=(const Vector& a) {
    int* p = new int[a.sz];
    for (int i = 0; i < sz; i++)
        p[i] = a.elem[i];
    delete[] elem;
    this->elem = p;
    this->sz = a.sz;
    return *this;
}

To have the strong exception guarantee you need to do the assignment in three distinct phases.

1. Make a copy of the RHS

   int* p = new int[a.sz];
   for (int i = 0; i < a.sz; i++) ////QUESTION 3
       p[i] = a.elem[i];
   

2. Replace the content of the current object using exception safe NO THROW techniques.

   std::swap(this->elem, p);  // use swap rather than assignment (see below)
   this->sz = a.sz;
   

3. Dealocate the old object.

   delete[] p; // Note it was swapped above.
   

Note: We do the deallocation after updating the object to a consistent state. This is because the deallocation may fail (or throw an exception). So if you deallocate before your object is consistent you leave your object in an invalid state that is not usable.

Luckily for you int does not throw exceptions on deallocation (but a user defined type may do). So you should be careful. If another program comes along behind you and tries to make your Vector generic but does not notice this he may get screwed over accidently.

Looking at your function again:

Vector& Vector::operator=(const Vector& a) {
    int* p = new int[a.sz];
    for (int i = 0; i < sz; i++)
        p[i] = a.elem[i];
    delete[] elem;             // Assume your vector is not int but a user type.
                               // Deleting the array here will call the destructor
                               // on all the elements. Which may result in an exception.
                               // If this happens you do not know the state of `elem`
                               // but your object refers to it.
                               // So you have a dangling pointer.
                               // and because of the exception the rest of the code is
                               // not executed and you have an object in an invalid state.
                               //
                               // Also note if this happens you leak `p`
    this->elem = p;
    this->sz = a.sz;
    return *this;
}

There is also another technique to resolve all these problems. Its called the "Copy and Swap Idiom".

Vector& Vector::operator=(Vector a) // Pass by value so you get a copy.
{                                   // You were making a copy anyway.
                                    // This just makes a copy in a way that can't
                                    // leak if there is an exception.

    a.swap(*this);                  // Swap the content of a and this.
                                    // Swap is a no-throw operation so totally safe.
                                    // The old data from `this` is now inside `a`
    return *this;
}                                   // When `a` goes out of scope at the end of the
                                    // function it calls the destructor and tides up
                                    // any allocated memory (remember the old this data
                                    // is now inside `a` and thus gets correctly deleted).
                                    // And the whole thing is exception safe.
                                    // And much shorter.

 void swap(Vector& other) nothrow
 {
     std::swap(elem, other.elem);
     std::swap(sz,   other.sz);
 }

Returning const bool does not make any sense.

const bool Vector::operator==(Vector& right) const { ////QUESTION 4

You suffer from the same problem here as you did in the assignment operator.

Vector& Vector::operator+=(const Vector& a) {
    int* p = new int[sz + a.sz];
    for (int i = 0; i < sz; i++)
        p[i] = elem[i];
    for (int i = sz, ctr = 0; i < sz + a.sz; i++, ctr++)
        p[i] = a.elem[ctr];
    delete[] elem;
    this->elem = p;
    this->sz += a.sz;
    return *this;
}

I would rewrite the above as:

Vector& Vector::operator+=(const Vector& other) {
{
     Vector   newValue(sz + a.sz);   // Make a new object to hold the tmp data.
                                     // This makes sure that there is no leaks
                                     // if there are exceptions.

     // Now copy the data into the new object
     std::copy(this->elem, this->elem + this->sz, newValue.elem);
     std::copy(other.elem, other.elem + other.sz, newValue.elem + this->sz);

     // Now Swap the newValue with the current object.
     newValue.swap(*this);
}// Destructor handles the deallocation.

Not sure this makes sense for a vector. Personally I would remove this function completely.

const Vector& Vector::operator++() {
    this->pushBack(0);
    return *this;
}

Just like the operator++ this make no sense. Remove this function.

const Vector& Vector::operator--() {
    //delete elem[sz - 1]; //delete (elem+sz); //this hates me.
    this->sz -= 1;
    return *this;
}

Interesting concept. (const Vector makes no sense on a return type (return by value)).

const Vector Vector::operator+(const Vector& a) {
    if (this->sz != a.sz)
        return NULL;

    Vector v = sz; //init's Vector with all 0's ( O(2n) with this init, and the for loop below..)
    for (int i = 0; i < this->sz; i++)
        v.elem[i] = elem[i] + a.elem[i];
    return v;
}

This should not compile:

   if (this->sz != a.sz)
        return NULL;

If it does. It is not doing what you think. It is calling a constructor that will convert the NULL into a Vector object. If this is happening I would try and find which one and make that constructor explicit so the compiler can't do that. Because it is probably not doing anything good.

This is not doing quite what you think.

Vector v = sz;

This is the same as:

Vector v = Vector(sz);

Which is the same as:

Vector v(Vector(sz));

So you are constructing a Vector object then using the copy constructor to copy the temporary vector into your new vector v. Luckily for you the compiler is allowed to optimzie that heavily and you probably only get one vector construction. But I would change the declaration to

Vector v(sz);  // Much clearer.

Your comment:

//init's Vector with all 0's ( O(2n) with this init, and the for loop below..)

A valid concern. I actually fixed this problem above. But in situations where that is not possible. Then I would have added another constructor that took two Vectors and added their content. I would just made the constructor private so that only the operator+ could use it.

Vector Vector::operator+(const Vector& a) {
    if (this->sz != a.sz)
        throw AnExceptionThatIsAppropriate("Plop");

    return Vector v(*this, a);
}
private:
    Vector(Vector const& lhs, Vector const& rhs)
       : elem{new int[lhs.sz]}
       , sz{lhs.sz}
    {
        std::transform(lhs.elem, lhs.elem + sz,
                       rhs.elem,
                       elem,
                       std::plus);
    }

Not sure. Why you want to use operator+ to add elements. Seems a bit of a stretch.

const Vector& Vector::operator+(int x) {
    this->pushBack(x); //recycling working code
    return *this;
}

But OK. lets use it (just as a demo case). In this case I would not make the result Vector const& as this prevents further mutation. Just return a reference to the Vector and it will allow you to chain operators.

Vector   mine;
mine + 5;       // Now vector has 5 in it.
mine + 6 + 7;   // Fails. As the result of `mine + 6` is a reference to a const Vector.
                // If you change it to return a reference it allows you add multiple values.

std::vector<int>  st;
st.push_back(5);
st.push_back(6).push_back(7); // Chained operators.

Yes this is really ineffecient as you are creating multiple arrays and copying stuff around. I would create a new private constructor to solve the issue.

Vector Vector::softRest() const {
    Vector v = *this;
    int* p = new int[v.sz - 1];
    for (int i = 0; i < sz - 1; i++)
        p[i] = v[i + 1];
    delete[] v.elem;
    v.elem = p;
    v.sz -= 1;
    return v;
} //seems wildly inefficient, suggestions?

Got bored.