C++ Operator overloading - 'recreating the Vector'

Question

I am currently in a collage second level programing course... We are working on operator overloading... to do this we are to rebuild the vector class... I was building the class and found that most of it is based on the [] operator. When I was trying to implement the + operator I run into a weird error that my professor has not seen before (apparently since the class switched IDE's from MinGW to VS express...) (I am using Visual Studio Express 2008 C++ edition...)

Vector.h

#include <string>
#include <iostream>
using namespace std;

#ifndef _VECTOR_H
#define _VECTOR_H

const int DEFAULT_VECTOR_SIZE = 5;

class Vector
{
private:
    int *   data;
    int     size;
    int     comp;
public:
    inline  Vector      (int Comp = 5,int Size = 0) 
        : comp(Comp), size(Size)    { if (comp > 0) { data = new int [comp]; } 
                                      else { data = new int [DEFAULT_VECTOR_SIZE];
                                      comp = DEFAULT_VECTOR_SIZE; }
                                    }
    int      size_      ()          const       { return size; }
    int      comp_      ()          const       { return comp; }
    bool     push_back  (int);
    bool     push_front (int);
    void     expand     ();
    void     expand     (int);
    void     clear      ();
    const    string at  (int);
    int&         operator[ ](int);
    int&         operator[ ](int) const;
    Vector&  operator+  (Vector&);
    Vector&  operator-  (const Vector&);
    bool     operator== (const Vector&);
    bool     operator!= (const Vector&);

    ~Vector() { delete [] data; }
};

ostream& operator<< (ostream&, const Vector&);

#endif

Vector.cpp

#include <iostream>
#include <string>
#include "Vector.h"
using namespace std;

const string Vector::at(int i) {
    this[i];
}

void Vector::expand() {
    expand(size);
}

void Vector::expand(int n ) {
    int * newdata = new int [comp * 2];
    if (*data != NULL) {
        for (int i = 0; i <= (comp); i++) {
            newdata[i] = data[i];
        }
        newdata -= comp;
        comp += n;
        data = newdata;
    delete newdata;
    }
    else if ( *data == NULL || comp == 0) {
        data = new int [DEFAULT_VECTOR_SIZE];
        comp = DEFAULT_VECTOR_SIZE;
        size = 0;
    }
}

bool Vector::push_back(int n) {
    if (comp = 0) { expand(); }
    for (int k = 0; k != 2; k++) {
        if ( size != comp ){
            data[size] = n;
            size++;
            return true;
        }
        else {
            expand();
        }
    }
    return false;
}

void Vector::clear() {
    delete [] data;
    comp = 0;
    size = 0;
}
int& Vector::operator[] (int place) { return (data[place]); }
int& Vector::operator[] (int place) const { return (data[place]); }

Vector& Vector::operator+ (Vector& n) {
    int temp_int = 0;

    if (size > n.size_() || size == n.size_()) { temp_int = size; }
    else if (size < n.size_()) { temp_int = n.size_();  }

    Vector newone(temp_int);
    int temp_2_int = 0;

    for ( int j = 0; j <= temp_int && 
                     j <= n.size_() && 
                     j <= size; 
                                        j++) {
        temp_2_int = n[j] + data[j];
        newone[j] = temp_2_int;
    }
////////////////////////////////////////////////////////////
    return newone;
////////////////////////////////////////////////////////////
}

ostream& operator<< (ostream& out, const Vector& n) {
    for (int i = 0; i <= n.size_(); i++) {
////////////////////////////////////////////////////////////
        out << n[i] << " ";
////////////////////////////////////////////////////////////
    }
    return out;
}

Errors:

out << n[i] << " "; error C2678:

binary '[' : no operator found which takes a left-hand operand of type 'const Vector' (or there is no acceptable conversion)

return newone;

error C2106: '=' : left operand must be l-value

---

As stated above, I am a student going into Computer Science as my selected major I would appreciate tips, pointers, and better ways to do stuff :D

Answer 1

This:

int operator[ ](int);

is a non-const member function. It means that it cannot be called on a const Vector.

Usually, the subscript operator is implemented such that it returns a reference (if you return a value, like you are doing, you can't use it as an lvalue, e.g. you can't do newone[j] = temp_2_int; like you have in your code):

int& operator[](int);

In order to be able to call it on a const object, you should also provide a const version of the member function:

const int& operator[](int) const;

Since you ask for "tips, pointers, and better ways to do stuff:"

• You cannot name your include guard _VECTOR_H. Names beginning with an underscore followed by a capital letter are reserved for the implementation. There are a lot of rules about underscores.
• You should never use using namespace std in a header.
• Your operator+ should take a const Vector& since it is not going to modify its argument.
• Your at should return an int and should match the semantics of the C++ standard library containers (i.e., it should throw an exception if i is out of bounds. You need to use (*this)[i] to call your overloaded operator[].
• You need to learn what the * operator does. In several places you've confused pointers and the objects to which they point.
• Watch out for confusing = with == (e.g. in if (comp = 0)). The compiler will warn you about this. Don't ignore warnings.
• Your logic will be much simpler if you guarantee that data is never NULL.

Answer 2

Besides of what others already wrote about your operator[]():

Your operator+() takes the right-hand side per non-const reference - as if it would attempt to change it. However, with A+B everyone would expect B to remain unchanged.

Further, I would implement all binary operators treating their operands equally (i.e., not changing either of them) as non-member functions. As member functions the left-hand side (this) could be treated differently. (For example, it could be subjected to overwritten versions in derived classes.)

Then, IME it's always good to base operator+() on operator+=(). operator+=() does not treat its operands equally (it changes its left one), so it's best done as a member function. Once this is done, implementing operator+() on top of it is a piece of cake.

Finally, operator+() should never, never ever return a reference to an object. When you say A+B you expect this to return a new object, not to change some existing object and return a reference to that.

Answer 3

There are so many errors in your code that it is hard to know where to start. Here's one:

 delete [] data;
 *data = *newdata;

You delete a pointer and then immediately dereference it.

And:

const string Vector::at(int i) {
    this[i];
}

This is (I think) a vector of ints. why is this returning a string? And applying the [] operator to this does not call your operator[] overload - it treats this as an array, which it isn't.

Answer 4

int Vector::operator[] (int place) { return (data[place]); }

This should be

int Vector::operator[] (int place) const { return (data[place]); }

so that you will be able to do the [] operation on const vectors. The const after the function declaration means that the class instance (this) is treated as const Vector, meaning you won't be able to modify normal attributes. Or in other words: A method that only has read access to attributes.

Answer 5

Can't fit this into a comment on Neil's answer, so I'm gonna have to go into more detail here.

Regarding your expand() function. It looks like this function's job is to expand the internal storage, which has comp elements, by n elements, while maintaining the size of the Vector. So let's walk through what you have.

void Vector::expand(int n) {
    int * newdata = new int [comp * 2];

Okay, you just created a new array that is twice as big as the old one. Error: Why doesn't the new size have anything to do with n?

    if (*data != NULL) {

Error: *data is the first int element in your array. It's not a pointer. Why is it being compared to NULL?

Concept Error: Even if you said if (data != NULL), which could be a test to see if there is an array at all, at what point in time is data ever set to NULL? new [] doesn't return NULL if it's out of memory; it throws an exception.

        for (int i = 0; i <= (comp); i++) {
            newdata[i] = data[i];
        }

Warning: You're copying the whole array, but only the first size elements are valid. The loop could just run up to size and you'd be fine.

        newdata -= comp;

Error: Bad pointer math. newdata is set to a pointer to who knows where (comp ints back from the start of newdata?!), and almost certainly a pointer that will corrupt memory if given to delete [].

        comp += n;

This is fine, for what it is.

        data = newdata;
        delete newdata;
    }

Error: You stored a pointer and then immediately deleted its memory, making it an invalid pointer.

    else if ( *data == NULL || comp == 0) {
        data = new int [DEFAULT_VECTOR_SIZE];
        comp = DEFAULT_VECTOR_SIZE;
        size = 0;
    }
}

Error: This should be in your constructor, not here. Again, nothing ever sets data to NULL, and *data is an int, not a pointer.

What this function should do:

• create a new array of comp + n elements
• copy size elements from the old array to the new one
• delete the old array
• set data to point to the new array

Good luck.