Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm new to C++ and am doing the C++ 4th Edition Stroustrup book. I expanded on one of the examples, and have a few questions to ask (embedded within the code: ////QUESTION 1-9).

Please provide any critiques, I'd like to bang in safe practice as early as possible. The Vector.h file simply declares these functions, and two private members, elem (list of elements, int*) and sz (the size, int).

Vector::Vector(std::initializer_list<int> list) //called via list init: ie, Vector v = {1, 2, 3, 4};
    :elem{ new int[list.size()] }, sz{ list.size() }
{
    copy(list.begin(), list.end(), elem); //copy list from start to end to elem

}

Vector::Vector(int s) //Constructor w/ size
{
    if (s < 0) throw length_error{"Vector::Vector"};
    elem = new int[s];
    for (int i = 0; i < s; i++)
        elem[i] = 0; //init elems to 0
    sz = s;
}

Vector::Vector(const Vector& a) //copy constructor - rule of 3 (if destructor then copy constructor & copy assignment op)
    :elem{ new int[sz] },
    sz{ a.sz } ////QUESTION 1
{
    for (int i = 0; i < sz; i++)
        elem[i] = a.elem[i];
}

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

Vector::~Vector() { //DESTRUCTOR
    cout << "DESTRUCTOR TRIGGERED! END OF DAYS COMING\n";
    delete[] elem;
}

int& Vector::operator[](int i) const { ////QUESTION 2
    if (i<0 || i>=size()) throw out_of_range{ "Vector::operator[]" };
    return elem[i];
}

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


const bool Vector::operator==(Vector& right) const { ////QUESTION 4
    if (size() != right.size())
        return false;
    else {
        for (int i = 0; i < size(); i++){ //left and right have same size, doesn't matter which
            if (elem[i] != right[i])
                return false;
        }
    }
    return true;
} ////QUESTION 5

//MEMSAFE (or so I like to think?)
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;
}

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

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

const Vector Vector::operator+(const Vector& a) { ////QUESTION 7
    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;
}

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

//Returns a Vector with the calling Vector's remaining elem's (ie, all except last) - doesn't affect calling  Vector in any way
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?

//Sets the calling Vector to be all elem's except last.
const Vector& Vector::hardRest() {
    int* p = new int[sz - 1];
    for (int i = 0; i < sz - 1; i++)
        p[i] = elem[i + 1];
    delete[] elem;
    this->elem = p;
    this->sz -= 1;
    return *this;
}


const Vector& Vector::pushBack(int x) {
    int* temp = new int[sz + 1];
    for (int i = 0; i < sz; i++)
        temp[i] = elem[i];
    temp[sz] = x; //temp is new int[sz+1]; so temp[sz] = last elem
    delete[] elem; ////QUESTION 8
    this->elem = temp;
    this->sz += 1;
    return *this;
}

//MEMSAFE
void Vector::addToEnd(std::initializer_list<int> list) {
    Vector v(list);
    this->operator+=(v);
}

int Vector::size() const {
    return this->sz; ////QUESTION 9
}

Questions (for ease of reading, they still have their placeholders in the code to let you know what I'm referring to):

  1. 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).

  2. removed

  3. 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;

  4. 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?

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

  6. 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?

  7. 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?

  8. 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?

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

EDIT:

Class declaration (Vector.h) -

#include <initializer_list>
#include <iostream>
#include <stdexcept>
using namespace std;

class Vector {
public:
    //Constructors
    Vector(std::initializer_list<int>); //constructor with {x, y, z} init (ie Vector v({1, 2, 3}); )
    Vector(int); //declare size, and initialize all elements to 0
    Vector();   //empty constructor
    Vector(const Vector& a); //COPY CONSTRUCTOR - rule of 3
    ~Vector(); //DESTRUCTOR - rule of 3

    //Overloaded operators
    int& operator[](int) const; //function type: int& because returns a[i] (or &a[i])
    const Vector& operator++();
    const Vector& operator--();
    const bool operator==(Vector&) const;
    Vector& operator=(const Vector&); //COPY ASSIGNMENT - rule of 3
    Vector& operator+=(const Vector&); //a Vector, += a vector (since its IN the vector class)
    const Vector operator+(const Vector&); //adds values of two equal sized vectors
    const Vector& operator+(int);       //deals with adding a single int (essentially .pushBack(int))

    //Input functions
    const Vector& pushBack(int); //add single element to end
    void addToEnd(std::initializer_list<int>); //add list to end
    Vector softRest() const;
    const Vector& hardRest();

    //Output functions
    int size() const;
    void arr_print() const;


private:
    int sz;
    int* elem;
};
share|improve this question
1  
Could you please move the questions outside of the code? Having them commented in makes it a little harder to read the code. –  Jamal May 17 at 0:11
    
Hmmm could you add the page/chapter numbers where we can find the declarations of those functions, e.g. operator[]? I can't find them in 13.6.1 (pp.375) –  dyp May 17 at 0:12
    
Fixed, sorry about that! I also added the page number (55) to the function in the 4th Edition Stroustrup book. However, I removed the question because the const was my own useless addition. –  Jhomas Tefferson May 17 at 1:54
    
Please add your class declaration (its important). –  Loki Astari May 17 at 15:37
    
Added. So sorry I didn't add that earlier! –  Jhomas Tefferson May 29 at 21:39

4 Answers 4

up vote 3 down vote accepted

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.

share|improve this answer
    
"Always prefer to use the initializer list" Maybe that's a pet peeve of mine, but I find that term ambiguous. To me, it's clear from the context that you're referring to the constructor's member-initializer-list, but OP also uses an initializer-list constructor (i.e. one that takes an std::initializer_list). –  dyp May 17 at 17:43
    
@dyp: Fixed (by removing). –  Loki Astari May 17 at 17:46
    
"This is because the deallocation may fail (or throw an exception)." Huh?? IIRC all StdLib types require UDTs not to throw exceptions in their dtor. I consider types that do evil. C++11 makes dtors noexcept(true) by default, anyway. -- your suggested fix (copy+swap) also just moves the "problem" into the dtor of Vector as far as I can see. –  dyp May 17 at 17:48
    
Ad Vector::operator+= -- std::copy_n, arguably. –  dyp May 17 at 17:52
    
I consider types that do evil.: I think we all do. But not everybody is perfect and we should program to anticipate for it (assuming it does not add much effort). IIRC all StdLib types require UDTs not to throw exceptions in their dtor. Is this new in C++11. It was not a requirement before. The Std libs behaved well if UDT threw an exception but did not guarantee to continue destroying all members if they did. –  Loki Astari May 17 at 17:53

Rather than try to directly answer your specific questions, I'm going to consider the basic approach you've taken.

At least in my opinion, this basic approach to a vector-like class is basically broken. The problem is fairly simple: you end up re-allocating your buffer every time you (for example) add an item to the collection. To go with that, you copy all the data every time as well.

You haven't included enough code to compile it easily, so I haven't tried to benchmark them, but I'd expect this to be substantially slower that something like std::vector for code like this:

for (int i=0; i<limit; i++)
    your_vector.pushBack(i);

With std::vector, this operation has linear complexity. With your Vector, it has quadratic complexity. This will be reasonably fast if limit is small (e.g., 10 items) but slow down dramatically as limit gets larger.

That difference mostly arises from differences in how the two manage memory. Your class always allocates exactly as many items as it's currently storing. When you add one item, it allocates space for one more item and copies all the existing items to the new buffer. In code like the loop above, that means your_vector[0] will be copied limit times, your_copy[1] will be copied limit-1 times, and so on.

std::vector instead uses a geometric growth pattern. It might start with room for, say, 10 items. If you add another item when it's full, it doubles the size of the buffer1. As the size grows, that limits allocation and copying dramatically.

Doing that, however, requires that you manage things rather differently though. Right now, you're using new to allocate objects (ints, in this case). To manage memory more reasonably, you want (need, really) to allocate "raw" memory, and then create objects in that memory as they're added.

To do that, you normally want to allocate the memory using operator new (which is pretty much malloc with a different name). You then use "placement new" to create objects in-place in that memory. To destroy an object, you directly invoke its destructor.

To go with that, you need to add a little bit of extra book-keeping information: along with the size that's currently in use, you normally want to store the size that's currently allocated, so you'll know when to do a re-allocate and copy cycle.


  1. Most real implementations actually use a factor of 1.5, but this doesn't change the basic idea.
share|improve this answer
    
"this operation has linear complexity." amortized ;) –  dyp May 17 at 17:37
    
"To manage memory more reasonably, you want (need, really) to allocate "raw" memory, and then create objects in that memory as they're added." Correct, but OP is using an int vector, where there's not much difference between operator new and the underlying allocation function. –  dyp May 17 at 17:39
    
@dyp: amortized: true, but mostly irrelevant. If you want truly linear complexity, you can do that easily enough (though it generally loses some efficiency overall). As far as raw memory goes, true enough for int. –  Jerry Coffin May 17 at 17:53
    
Sorry for taking such a long time, I was out of the country. I just got back and sat down to read everything. This is an incredibly simple but genius addition to increase the efficiency of the custom Vector. Thank you! :) –  Jhomas Tefferson May 29 at 21:22
copy(list.begin(), list.end(), elem); //copy list from start to end to elem

Should use std::copy. It might or might not work without the std:: depending on the type of the iterators (ADL) and any using-directives / using-declarations.

Note that the copy here and the initializations in other constructors cannot fail because the vector only stores ints. For complex data types, if those operations fail, you leak the newed memory. To avoid that, you either had to add a try-catch clause, or use the revised design from pp. 379 where Stroustrup introduces a base class / data member to deal with the memory management. In case of an exception in the constructor, the fully created subobjects will be destroyed, but the constructor of the class itself will not be called.

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

Avoid "raw" loops outside of algorithms. Here, you can use std::uninitialized_fill or std::fill_n (for ints).

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

This is only correct if you use the upper bound a.sz, since that's the size of your buffer. Again, you can replace the loop by a standard algorithm such as std::copy_n or std::copy.

Similarly, you can use std::equal for the equality operator.

The assignment-operator can be optimized for the case this->sz == a.sz, since you don't need to allocate another buffer in that case. Typically, a vector class also supports a size different from the capacity, i.e. the number of elements stored in the vector being smaller than the size of the buffer. In that case, the optimization works for this->sz >= a.sz and could also be applied to operator+=.

Your operator++ and operator-- are not very intuitive in my humble opinion. Additionally, for some Vector v and Vector w, w = w + v and w += v do something quite different. Similarly, v = v + 1 vs. ++v. You should make those operators consistent.

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

In my opinion, the sizes of those two vectors not being equal is an error condition. Also, return NULL will use NULL as the integer 0 and call your constructor Vector(int) returning a 0-size vector. In my opinion, that's surprising.

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

Exactly. You should avoid the init. For example by using a (private?) constructor that does not perform any initialization of the array.

The softReset function allocates twice and creates a vector with all but the first element, as opposed to the comment (as does hardReset).

const Vector& Vector::pushBack(int x)

This uses a very inefficient reallocation strategy. Take a look at vector::push_back on pp. 385


const correctness

I think you can hardly overuse const. The general rule (nowadays) is rather: Make everything const by default unless you need to modify it.

You should not return a non-const reference from a const member function:

int& Vector::operator[](int i) const

->

int const& Vector::operator[](int i) const

This requires adding a second overload

int& Vector::operator[](int i)

The idea is that you can access the elements of a const Vector, but you may not change them. OTOH, if the Vector is not const, you may change them. Example:

Vector v0(10);
v0[1] = 42; // use non-const operator[] to change elements

Vector const v1(10); // Vector const == const Vector
v1[1] = 42; // should not be allowed, the vector is const
std::cout << v1[1]; // should be allowed, does not modify the vector
                    // requires a const member function

By having two functions:

int      & Vector::operator[](int i)
int const& Vector::operator[](int i) const

You can allow both: modifying a non-const vector, and observing the values of a const vector. One could argue that an int* const (a constant pointer to a mutable int) is different from an int const* (a mutable pointer to a constant int). But your vector including its copy constructor follows value semantics: it behaves to the outside as if its contents are a collection of values, like an array of integers. Therefore, a Vector const should not allow changing the elements, similar to a int const does not allow changing the stored value.

const bool Vector::operator==(Vector& right) const

This should be a non-member friend function:

bool operator==(Vector const& left, Vector const& right)

The reason is that == is symmetric, and therefore (as a general rule) should allow the same conversions for both arguments. Since you don't modify the argument, make it const. If you return by value, don't return a const value.

Speaking of conversions, you should make the converting constructor Vector(int) explicit: It is an expensive conversion that shouldn't easily be triggered. Currently, you can do things like Vector v(42); if(v == 103) { ... } which is rather nonsensical.

const Vector& Vector::operator++()

Per convention, operator++ being a non-const member function returns a non-const reference (so you can chain other non-const member function calls to the return value, even if that's ugly). Similarly, hardReset and pushBack.


code duplication

There are a lot of calls to new and delete[] in your code. You can remove most of them by using local Vectors or by introducing another class that only deals with the memory management, and using local instances of that class.

share|improve this answer
    
By creating a local Vectors (to play with) within the Vector class, will my Destructor automatically deal with its destruction? Also, could you explain why int const& Vector::operator[](int i) const would require a second operator overload? –  Jhomas Tefferson May 29 at 21:33
    
"By creating a local Vectors (to play with) within the Vector class" Do you mean inside member functions? Yes, their destructors will be called and can deal with destruction -- if you're not trying to create those locally in the destructor, of course (that leads to recursion). I'll add an explanation why that operator is beneficial to my answer. –  dyp May 29 at 21:45
    
@JhomasTefferson Example + some explanation added. Hope that helps. –  dyp May 29 at 21:53
    
Yeah, I meant member func's, sorry - long day hah. Thank you, this definitely clears things up! –  Jhomas Tefferson May 31 at 22:23

I'll go ahead and answer most of the questions. I'll also add some additional notes.

  1. 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).

    This article explains several reasons why initializer lists are preferred. Depending on the use, initializer lists are mandatory. They also have benefits such as supporting the same constructor parameter and member names, and allowing const data members to be initialized.

  2. Removed

  3. 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;.

    There's really no significant difference between sz and size(). The ones with this-> are unnecessary because, in general, this-> is redundant when used with data members. I'd stay with sz here.

  4. 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?

    There's generally no harm in over-using const when ensuring that data members cannot be mutated unknowingly. It's also helpful to (human) readers as it indicates that something should read but not mutate data members.

  5. Skipped

  6. Skipped

  7. 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?

    As the function doesn't change the argument at all, a should remain const. Doing so will prevent it from being changed accidentally, and it's usually cheaper to pass by const& if the argument is large (usually with user-defined types).

  8. Skipped

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

    I don't think it's relevant to multithreading. As mentioned before, this-> is not necessary here, and it's alright to just use return sz.

Additional notes:

  • It's okay to start with an int vector, but eventually you should consider making it templated. This will allow any type to use used for this implementation, not just ints.

  • size() could be defined in the class definition, where it will be inlined (anything inside the class declaration is inlined). It's common to do with with simple accessors.

  • The output in the destructor is just noisy, unless you're just keeping it there temporarily to indicate that it has been invoked. I just want to make you aware of that anyway.

  • For maintenance reasons, it would be preferred to use curly braces for loops and conditionals, even if they have just one line. This is helpful in case additional lines need to be added.

share|improve this answer
    
Question 3 related to the assignment operator, where only a.sz is correct. –  dyp May 17 at 12:45
    
I'm actually in the process of making it templated, I will eventually post the updated version and ask for more critique. This is incredible. I've been programming for a few years, and I'm finishing up my Computer Science degree, however, I've never had anyone ever provide me with such detailed feedback. You guys are glorious! –  Jhomas Tefferson May 29 at 21:34
    
@JhomasTefferson: You're welcome! I too am finishing up my CS degree, but I've learned most of my C++ online. Getting a code review and/or reviewing code can really give you an advantage. –  Jamal May 29 at 21:37
    
Best of luck to the both of us on finishing the degree! We did a lot of Python/Java, one C course, and not a single C++ course. So it's up to me to learn it. It's an incredibly powerful language. Also! As for the noise in the destructor, thanks to Java's automatic garbage collection, I've never really had experience with destructors. The output in it is noisy, but was a pleasant sight to see when learning how they work/when objects are destructed, etc. Again, thanks for everything! –  Jhomas Tefferson May 31 at 22:03

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.