Assessment of my class definition/implementation

[fibbo]

Hey guys! I hope what I'm about to do is not frowned upon :)

I just would like some input/critics/heads up on my class definition and implementation I did for my project. I am by no means a trained programmer and I doubt my coding style is very good so I thought it might be a good idea to get some inputs on something I put some time and effort in to create with my best knowledge.

I created a class PNVector that allows to me to create an N-sized vector on which I overloaded several operations. Apart from an assessment I also have one question. As you can see I overloaded * and / so I can multiply/divide the vector by a scalar. Now naturally only PNVector*double works but double*PNVector does not. Is the only solution to make this work to overload the binary * and / respectively?

For any answer/input I'm thankful!

Definitions here:

#ifndef RK4_project_PNVector_hpp
#define RK4_project_PNVector_hpp
#include <vector>

class PNVector {
public:
    PNVector(int N){
        m_vector = new std::vector<double>(N);
        for (unsigned int i = 0; i<m_vector->size(); ++i) {
            m_vector->at(i) = 0;
        }
    }
    ~PNVector(){
        delete m_vector;
    }
    
    /*     copy constructor      */
    PNVector(const PNVector &vSource)
    {
        m_vector = new std::vector<double>(vSource.m_vector->size());
        for (unsigned int i = 0; i<vSource.m_vector->size(); ++i) {
            m_vector->at(i) = vSource.m_vector->at(i);
        }
    }

    PNVector operator+(const PNVector &rhs);
    PNVector operator+(const double h);
    PNVector operator*(const double h);
    PNVector operator/(const double h);
    PNVector operator=(const PNVector &other);
    double &operator[](unsigned int index);
    PNVector operator*(PNVector &rhs);

    
    void Print();

private:
        std::vector<double> *m_vector;
protected:
};


#endif

Implementations here:

#include <iostream>
#include "PNVector.hpp"

PNVector PNVector::operator+(const PNVector &rhs) {
    PNVector vector(*this);
    for (unsigned int i = 0; i<this->m_vector->size(); ++i) {
        vector.m_vector->at(i) += rhs.m_vector->at(i);
    }
    return vector;
}

PNVector PNVector::operator+(const double h) {
    PNVector vector(*this);
    for (unsigned int i = 0; i<this->m_vector->size(); ++i) {
        vector.m_vector->at(i) += h;
    }
    return vector;
}

double &PNVector::operator[](unsigned int index) {
    return this->m_vector->at(index);
}

PNVector PNVector::operator*(const double h) {
    PNVector vector(*this);
    for (unsigned int i = 0; i<this->m_vector->size(); ++i) {
        vector.m_vector->at(i) *= h;
    }
    return vector; 
}
// Inner product
PNVector PNVector::operator*(PNVector &rhs) {
    PNVector vector(*this);
    for (unsigned int i = 0; i<this->m_vector->size(); ++i) {
        vector.m_vector->at(i) *= this->m_vector->at(i);
    }
    return vector; 
}

PNVector PNVector::operator/(double h) {
    PNVector vector(*this);
    for (unsigned int i = 0; i<this->m_vector->size(); ++i) {
        vector.m_vector->at(i) /= h;
    }
    return vector; 
}

PNVector PNVector::operator=(const PNVector &other) {
    if (this!= &other) {
        std::vector<double>* new_m_vector = new std::vector<double>(other.m_vector->size());
        for (unsigned int i = 0; i<other.m_vector->size(); ++i) {
            new_m_vector->at(i) = other.m_vector->at(i);
        }
        delete m_vector;
        m_vector = new_m_vector;
    }
    return *this;
}

void PNVector::Print() {
    for (unsigned int i = 0; i<this->m_vector->size(); ++i) {
        std::cout << m_vector->at(i) << std::endl;
    }
}

[m4ster_r0shi]

I just would like some input/critics/heads up on my class definition and implementation I did for my project.

Looks good. There are a couple of small issues, but other than that looks good. About the small issues now...

Your m_vector doesn't have to be a vector pointer. A vector object would be fine. This would greatly simplify
some things. E.g. your assignment operator (which, by the way, should return a reference) would look like this:

PNVector & PNVector::operator=(const PNVector &other) {
    if (this!= &other)
        this->m_vector = other.m_vector;
    return *this;
}

The rest of your operators could use some const qualifiers.
E.g. PNVector operator+(const PNVector &rhs) [B]const[/B]; Don't forget that at() performs bounds checking, which is unnecessary
in many cases in your code. You could just use operator[] instead.

If you intend to also provide +=, *=, ... etc operators (which is a good idea), you
should implement +, *, ... etc in terms of +=, *=, ... and not the other way around.

I.e. it should look like this:

PNVector & PNVector::operator += (double val)
{
    // ...

    return *this;
}

PNVector PNVector::operator + (double val)
{
    PNVector ret(*this);

    ret += val;

    return ret;
}

Is the only solution to make this work to overload the binary * and / respectively?

Yes. You have to also write global operator overloads (note that both global
and member operators are binary operators, as they accept two arguments).

Finally, note that if this is only a helper class for your runge kutta project, you can always use std::valarray instead.

[ChaseRLewis]

When a vector is declared to have a certain number of things such as you init it to size(10000) it's very inefficient to init it with a for loop. Instead you can take a reference pointer to the first member than ZeroMemory it for it's size which is much quicker. Keep in mind this only works well if you have added/removed things. After that you have to worry because internally I believe pushing and erasing keeps items as a linked list but I never really have looked into it.

Overall roshi covered everything else. Though I don't see much advantage to using this over the raw std::vector<double>. Also if your worried about long lists and don't have concurrency issues declaring the size as a int before hand can save you some cycles as compilers typically don't deal with for loops and inline functions very well.

ZeroMemory(&m_vector[0],m_vector.size()*sizeof(double));

//same as above
memset(&m_vector[0],0,m_vector.size()*sizeof(double));

[Narue]

When a vector is declared to have a certain number of things such as you init it to size(10000) it's very inefficient to init it with a for loop

When the vector is already resized, using a loop to initialize is no different from using a loop to initialize an array. ZeroMemory() or memset() might be faster, but they gain that speed at a cost: throwing away type information and writing bytes in the most efficient manner possible.

The problem with this kind of optimization is many types don't facilitate that kind of type punning. This case is one of them, because memset() fills in all bytes of the object with 0, and that's not required to be an equivalent representation of 0 for type double.

The recommendation for low level stuff like memset(), bzero(), and ZeroMemory() is to restrict usage to arrays of the built-in integer types or POD aggregates of integer types. Anything else, including pointers, is unsafe.

Now for the real kicker: all of this is moot because std::vector's resizing constructor looks like this (simplified for posting):

vector(size_type n, const T& value = T());

When creating a sized vector with the constructor using only the size argument, all of the elements are initialized to the default value for their type. No further initialization is necessary:

std::vector<double> v(5); // All elements initialized to 0.0

[fibbo]

Back from a tough weekend hence the late reply (blame the vodka).

First: thanks to all the replies, they are very helpful and I appreciate it.

I have one more question about the use of const in operator overloading. The initial problem I had was that I couldn't chain operations.
a, q and p are NVectors.

a = q*2 + p/2

This didn't compile and delivered following error:

pvec_test.cpp:24: error: no match for ‘operator+’ in ‘PNVector::operator*(double)(2.0e+0) + PNVector::operator/(double)(2.0e+0)’
PNVector.hpp:34: note: candidates are: PNVector PNVector::operator+(PNVector&)
PNVector.hpp:35: note: PNVector PNVector::operator+(double)

Now adding a const in my operator+(PNVector) (i.e. operator+(const PNVector &rhs) fixed this issue and I fail to grasp why.

[Narue]

Now adding a const in my operator+(PNVector) (i.e. operator+(const PNVector &rhs) fixed this issue and I fail to grasp why.

Long story short, you were trying to bind a temporary object to a non-const lvalue reference (not legal). Temporaries may be bound to a const lvalue reference, which is why adding const fixed the problem.

[fibbo]

Thanks... any chance you can tell me where I can read the long story? :)

Thread solved in any case. Thanks to you all.

Btw, yes, at the moment I only see myself using this class for my runge kutta integrator and for that I was not efficient at all to implement my own class but I just took it as an opportunity to get some hands on training. After all, in my eyes learning by doing is one of the best ways to learn c++ (or any other language - except french, but thats a different story :D)