Sign in to follow this  
c4c0d3m0n

Please review my operator overloading

Recommended Posts

c4c0d3m0n    100
Hello all. I created a Vector class to teach myself one or two things. One of these things is operator overloading. Can you please review my code and tell me wether I've applied the overloading correctly? Thanks in advance. vector.h
///
/// vector.h
///
///


#ifndef VECTOR_H
#define VECTOR_H



// A Vector in space
struct Vector
{
public:

    double x;
    double y;
    double z;

    Vector operator- ();
    Vector operator* ( const double d );
    Vector operator/ ( const double d );
    Vector operator+= ( const Vector& v );
    Vector operator-= ( const Vector& v );
    Vector operator*= ( const double d );
    Vector operator/= ( const double d );
    void operator++ ();
    void operator-- ();

    // Get the length of a Vector
    double length();

    // Get the unit Vector with length 1
    Vector unit();

};

Vector operator+ ( const Vector& v1, const Vector& v2 );
Vector operator- ( const Vector& v1, const Vector& v2 );
double operator* ( const Vector& v1, const Vector& v2 );

// Return a Vector
Vector newVector( double x, double y, double z );

// Get the distance between two Vectors
double dist( Vector v1, Vector v2 );

// Create the cross product between a and b
Vector crossproduct( Vector a, Vector b );



# endif // VECTOR_H

vector.cpp
///
/// vector.cpp
///
///


#include "vector.h"
#include <cmath>



Vector operator+ ( const Vector& v1, const Vector& v2 )
{
    return newVector( v1.x + v2.x, v1.y + v2.y, v1.z + v2.z );
}

Vector operator- ( const Vector& v1, const Vector& v2 )
{
    return newVector( v1.x - v2.x, v1.y - v2.y, v1.z - v2.z );
}

double operator* ( const Vector& v1, const Vector& v2 )
{
    return v1.x*v2.x + v1.y*v2.y + v1.z*v2.z;
}

Vector Vector::operator- ()
{
    return newVector( -this->x, -this->y, -this->z );
}

Vector Vector::operator* ( const double d )
{
    return newVector( this->x * d, this->y * d, this->z * d );
}

Vector Vector::operator/ ( const double d )
{
    return newVector( this->x / d, this->y / d, this->z / d );
}

Vector Vector::operator+= ( const Vector& v )
{
    *this = *this + v;
    return *this;
}

Vector Vector::operator-= ( const Vector& v )
{
    *this = *this - v;
    return *this;
}

Vector Vector::operator*= ( const double d )
{
    *this = *this * d;
    return *this;
}

Vector Vector::operator/= ( const double d )
{
    *this = *this / d;
    return *this;
}

void Vector::operator++ ()
{
    *this += this->unit();
}

void Vector::operator-- ()
{
    *this -= this->unit();
}



// Return a Vector
Vector newVector( double x, double y, double z )
{
    Vector v;
    v.x = x;
    v.y = y;
    v.z = z;

    return v;
}


// Get the length of a Vector
double Vector::length()
{
    return sqrt( this->x*this->x + this->y*this->y + this->z*this->z );
}

Vector Vector::unit()
{
    return *this / this->length();
}


// Get the distance between two Vectors
double dist( Vector v1, Vector v2 )
{
    return sqrt( (v2.x-v1.x)*(v2.x-v1.x) + (v2.y-v1.y)*(v2.y-v1.y) + (v2.z-v1.z)*(v2.z-v1.z) );
}

// Create the cross product between a and b
Vector crossproduct( Vector a, Vector b )
{
    return newVector( a.y*b.z - a.z*b.y, a.z*b.x - a.x*b.z, a.x*b.y - a.y*b.x );
}



/// End of vector.cpp
///

Share this post


Link to post
Share on other sites
adam23    164
For your first pass I think you did a good job.

One thing I generally do is declare these operators...



Vector operator+ ( const Vector& v1, const Vector& v2 );
Vector operator- ( const Vector& v1, const Vector& v2 );
double operator* ( const Vector& v1, const Vector& v2 );

as ...

Vector operator+ ( const Vector& v1, const Vector& v2 ) const;
Vector operator- ( const Vector& v1, const Vector& v2 ) const;
double operator* ( const Vector& v1, const Vector& v2 ) const;

Share this post


Link to post
Share on other sites
Porthos    210

// Return a Vector
static Vector newVector( double x, double y, double z );

This should be static, shouldn't it? The same applies for the cross product.


Vector Vector::operator-= ( const Vector& v )

And this should return a const reference to a Vector, not a copy. Plus the const keyword at the end as proposed by adam:


const Vector& Vector::operator-= ( const Vector& v ) const


And you've forgotten to overload the ++/-- operators a second time:


void operator++ (int);
void operator-- (int);

I swer you, eveyone expects that both pre- and postfix increment/decrement operators are defined.

Best regards,
Porthos

Share this post


Link to post
Share on other sites
michael saad    122
Hello, one more thing to mention here is that you'd better set the return type to a reference to struct Vector to reduce time of copying the entire vector data.

Example:

Vector& Vector::operator-= ( const Vector& v )
{
*this = *this - v;
return *this;
}

This is a good thing as a practice though, since sometimes we take time to load our objects but it's not a requirement. :)

Share this post


Link to post
Share on other sites
Simian Man    1022
Quote:
Original post by adam23
For your first pass I think you did a good job.

One thing I generally do is declare these operators...



Vector operator+ ( const Vector& v1, const Vector& v2 );
Vector operator- ( const Vector& v1, const Vector& v2 );
double operator* ( const Vector& v1, const Vector& v2 );

as ...

Vector operator+ ( const Vector& v1, const Vector& v2 ) const;
Vector operator- ( const Vector& v1, const Vector& v2 ) const;
double operator* ( const Vector& v1, const Vector& v2 ) const;



Since they are free functions, they can't themselves be declared const. Only member functions can.

Share this post


Link to post
Share on other sites
Simian Man    1022
Quote:
Original post by Porthos

// Return a Vector
static Vector newVector( double x, double y, double z );

This should be static, shouldn't it? The same applies for the cross product.

Why should it be static? It's not a member function. I would suggest instead of having newVector, to simply have a constructor with arguments.

Quote:
Original post by Porthos

Vector Vector::operator-= ( const Vector& v )

And this should return a const reference to a Vector, not a copy. Plus the const keyword at the end as proposed by adam:


const Vector& Vector::operator-= ( const Vector& v ) const


OK this could at least syntactically be a const member function, but the function *changes* the calling object which would mean it's not a const function.

Share this post


Link to post
Share on other sites
adam23    164
Heres an example of how I declared my current class.



class Point2F
{
public:
Point2F();
~Point2F(){}

Point2F( float x, float y );
Point2F( const Point2F& right );

static void initScript( ScriptManager *script );

inline void normalize();
inline float len( ){ return sqrtf(x * x + y * y); }
inline float dot( const Point2F& );

Point2F& operator /= ( const float val );
Point2F& operator += ( const Point2F& );
Point2F& operator -= ( const Point2F& );
Point2F& operator *= ( const float val );

//binary operators
Point2F operator + ( const Point2F& ) const;
Point2F operator - ( const Point2F& ) const;
Point2F operator * ( const float ) const;
Point2F operator / ( const float ) const;
void operator = ( const Point2F& );


bool operator == ( const Point2F& ) const;
bool operator != ( const Point2F& ) const;

float x,y;
};




//============================================================
Point2F::Point2F( ) : x(0.0f), y(0.0f)
{}

//============================================================
Point2F::Point2F( float X, float Y ) : x(X), y(Y)
{}

//============================================================
Point2F::Point2F( const Point2F& right )
{
x = right.x;
y = right.y;
}

//============================================================
void Point2F::initScript( ScriptManager *script )
{
... Initialize for use in Script
}

//============================================================
void Point2F::normalize( )
{
float len = sqrtf(x * x + y * y);
x /= len;
y /= len;
}

//============================================================
float Point2F::dot( const Point2F &vec )
{
return x * vec.x + y * vec.y;
}

//============================================================
void Point2F::operator = ( const Point2F& right )
{
x = right.x;
y = right.y;
}

//============================================================
Point2F& Point2F::operator /=( const float val )
{
x /= val;
y /= val;
return *this;
}

//============================================================
Point2F& Point2F::operator -=( const Point2F &right )
{
x -= right.x;
y -= right.y;
return *this;
}

//============================================================
Point2F& Point2F::operator +=( const Point2F &right )
{
x += right.x;
y += right.y;
return *this;
}

//============================================================
Point2F& Point2F::operator *=( const float val )
{
x *= val;
y *= val;
return *this;
}

//============================================================
Point2F Point2F::operator +( const Point2F &right ) const
{
return Point2F(x + right.x, y + right.y);
}

//============================================================
Point2F Point2F::operator -( const Point2F &right ) const
{
return Point2F(x - right.x, y - right.y);
}

//============================================================
Point2F Point2F::operator *( const float val ) const
{
return Point2F(x * val, y * val);
}

//============================================================
Point2F Point2F::operator /( const float val ) const
{
return Point2F(x / val, y / val);
}

//============================================================
bool Point2F::operator ==( const Point2F &right ) const
{
return (x == right.x && y == right.y);
}

//============================================================
bool Point2F::operator !=( const Point2F &right ) const
{
return (x != right.x || y != right.y);
}

Share this post


Link to post
Share on other sites
adam23    164
Quote:
Original post by Simian Man
Quote:
Original post by Porthos

// Return a Vector
static Vector newVector( double x, double y, double z );

This should be static, shouldn't it? The same applies for the cross product.

Why should it be static? It's not a member function. I would suggest instead of having newVector, to simply have a constructor with arguments.

Quote:
Original post by Porthos

Vector Vector::operator-= ( const Vector& v )

And this should return a const reference to a Vector, not a copy. Plus the const keyword at the end as proposed by adam:


const Vector& Vector::operator-= ( const Vector& v ) const


OK this could at least syntactically be a const member function, but the function *changes* the calling object which would mean it's not a const function.



Your absolutely right I was just going to post the same thing.

Share this post


Link to post
Share on other sites
Simian Man    1022
Quote:
Original post by adam23
Yeah your right about that, strange he did it that way.


I actually quite prefer defining these operators as free functions. In general, it's good practice to only make member functions of the things that cannot be implemented in terms of the objects public interface.

<edit>
Also, if a class provides automatic type conversion, defining operators as member functions restricts the range of calls you can make as the following contrived example demonstrates:

This code does not work:

class Num
{
public:
// automatic type conversion
Num(int x) : n(x) {}

int getNum( ) const
{
return n;
}

// member
Num operator+(const Num& rhs) const
{
return n + rhs.n;
}

private:
int n;
};

int main( )
{
Num a(5);

// not allowed
Num sum = 7 + a;

return 0;
}

$ g++ temp.cpp
temp.cpp: In function 'int main()':
temp.cpp:28: error: no match for 'operator+' in '7 + a'




Whereas this code does:

class Num
{
public:
// automatic type conversion
Num(int x) : n(x) {}

int getNum( ) const
{
return n;
}

private:
int n;
};

// free
Num operator+(const Num& lhs, const Num& rhs)
{
return Num(lhs.getNum( ) + rhs.getNum( ));
}


int main( )
{
Num a(5);

// *is* allowed
Num sum = 7 + a;

return 0;
}

Share this post


Link to post
Share on other sites
Porthos    210
Oh, yes, I've also overseen it was a free function. And you're right ,operator-= should't be const, of course. Don't know what I thought before writing that ... However, it should return a reference to a Vector in every case.

Share this post


Link to post
Share on other sites
c4c0d3m0n    100
Hmm, I looked up what the static keyword does, and it doesn't seem to be what I am aiming for with my functions.
Quote:
(...) When modifying a variable or function at file scope, the static keyword specifies that the variable or function has internal linkage (its name is not visible from outside the file in which it is declared).


I'm hesitant to throw the const keyword around, for I can't find out what it does when I put it in different locations. Why should I return a constant reference instead of just a reference?


///
/// vector.h
///
///


#ifndef VECTOR_H
#define VECTOR_H



/// ////// ///
/// Vector ///
/// ////// ///

// A Vector in space
struct Vector
{
public:

double x;
double y;
double z;

Vector operator- ();
Vector operator* ( const double d );
Vector operator/ ( const double d );
Vector& operator+= ( const Vector& v );
Vector& operator-= ( const Vector& v );
Vector& operator*= ( const double d );
Vector& operator/= ( const double d );
void operator++ ();
void operator-- ();

// Get the unit Vector with length 1
Vector unit();

// Get the length of a Vector
double length();

};

Vector operator+ ( const Vector& v1, const Vector& v2 );
Vector operator- ( const Vector& v1, const Vector& v2 );
double operator* ( const Vector& v1, const Vector& v2 );

// Return a Vector
Vector newVector( double x, double y, double z );

// Create the cross product between a and b
Vector crossproduct( Vector a, Vector b );

// Get the distance between two Vectors
double dist( Vector v1, Vector v2 );



#endif // VECTOR_H

Share this post


Link to post
Share on other sites
gregrampage    177
Quote:
Original post by c4c0d3m0n
I'm hesitant to throw the const keyword around, for I can't find out what it does when I put it in different locations. Why should I return a constant reference instead of just a reference?


You shouldn't be hesitant to use const. The worst case when you use it incorrectly is the compiler gives you an error. Read more about const correctness here

Share this post


Link to post
Share on other sites
rip-off    10979
Quote:
Original post by c4c0d3m0n
Hmm, I looked up what the static keyword does, and it doesn't seem to be what I am aiming for with my functions.
Quote:
(...) When modifying a variable or function at file scope, the static keyword specifies that the variable or function has internal linkage (its name is not visible from outside the file in which it is declared).


That is one use of the keyword "static", but not the only one (the C++ standards body hate adding keywords to the language). However, that is only free functions, and is deprecated in favour of anonymous namespaces.

In a class or structure context, it defines a function that does not have an implicit "this" parameter. This means the function can no longer be called on an instance, like so:
someObject.someFunction()

Instead we call it using the class name:
SomeClass::someFunction()


The only difference between a static function and a free function (apart from namespace collisions) is that static functions have access to the private members of instances of its class. Since your class is all-public, there is no real difference, apart from stylistic issues.

Quote:

I'm hesitant to throw the const keyword around, for I can't find out what it does when I put it in different locations. Why should I return a constant reference instead of just a reference?


In the context of operator overloading, when you return *this return by const reference (except for operator=(), but you don't need that one). If you return a local object, return by value. Append "const" to the end of member functions that shouldn't modify the object (for example: your "operator-" doesn't modify "this", so it should be marked const).

I would make most of the operators free functions, because your class has public members. This is cleaner than making them const member functions (IMO).

My vector class only has a constructor, and a "length" member function (personal preference only, I like the look of vector.length() more than length(vector)).



General comments:

Since the cross product is a free function, as a client of this code I would expect that the dot product would be similar.

I've never really seen a use for ++ or -- when using vectors. I would be of the opinion: "unless you need it, don't bother writing it". Should you decide to keep them though, know that it is idiomatic for operator++ and operator-- to return *this in pre-version, and for post-version make a copy of the old value before modifying the members, and return it after the modifications.

Share this post


Link to post
Share on other sites
adam23    164
You don't need to use static for anything in your code.

If you look in my above code I have a function

static void initScript( ScriptManager *script ).


Then in my ScriptManager I call it like this.

Point2F::initScript( script );
Point3F::initScript( script );
RectF::initScript( script );
CObject::initScript( script );


Basically a static function is not technically owned by the class, so you don't need an object of that class to access it.

I've noticed a lot of large older codebases to use a lot of static lists to keep track of the entities in the world. This causes problems when moving that codebase to one that runs on multiple processors.

Share this post


Link to post
Share on other sites
c4c0d3m0n    100
Alright then, after reading everything, this is how I've created my Vector class:

///
/// vector.h
///
///


#ifndef VECTOR_H
#define VECTOR_H



/// ////// ///
/// Vector ///
/// ////// ///

// A Vector in space
struct Vector
{
public:

double x;
double y;
double z;

Vector operator- ();
Vector operator* ( const double d );
Vector operator/ ( const double d );
const Vector& operator+= ( const Vector& v );
const Vector& operator-= ( const Vector& v );
const Vector& operator*= ( const double d );
const Vector& operator/= ( const double d );
void operator++ ();
void operator-- ();

// Get the unit Vector with length 1
Vector unit();

// Get the length of a Vector
double length();

};

Vector operator+ ( const Vector& v1, const Vector& v2 );
Vector operator- ( const Vector& v1, const Vector& v2 );
double operator* ( const Vector& v1, const Vector& v2 );

// Return a Vector
Vector newVector( double x, double y, double z );

// Create the cross product between a and b
Vector crossproduct( Vector a, Vector b );

// Get the distance between two Vectors
double dist( Vector v1, Vector v2 );



#endif // VECTOR_H



I've left in the operator++ and operator-- for didactic purposes. I'll remove them once I decide to actually use this class.

Share this post


Link to post
Share on other sites
Simian Man    1022
The only things I would change would be (as Rip-Off said) to make the dot-product a named function instead of an operator, and to make it a class rather than a struct.

Of course using struct and class in this context are exactly the same, but it is more idiomatic to use the class keyword when using member functions and the struct keyword for POD types only.

Also if you ever need to forward declare this class, you would have to do it as struct Vector; which would be non-intuitive for most programmers.

Share this post


Link to post
Share on other sites
SiCrane    11839
Quote:
Original post by Simian Man
Also if you ever need to forward declare this class, you would have to do it as struct Vector; which would be non-intuitive for most programmers.


Types defined with struct can be forward declared with class and vice versa.

Share this post


Link to post
Share on other sites
jyk    2094
1. I would just go with cross() rather than crossproduct() (it's a little less verbose).
2. I'd suggest a named dot() function rather than using operator*() for this (as has already been mentioned).
3. ++ and -- should probably not be implemented for a vector class (I gather you already had planned to remove these, though).

Share this post


Link to post
Share on other sites
c4c0d3m0n    100
Alright, I went through all the tips once more, and I toko another good look at my code. I decided less is indeed more, so I got rid of useless stuff. I also threw the const keyword around a few more times and I changed some other stuff. This is the current standing of my Vector struct.

I decided to keep it as a struct, for I feel it's not much more than just a collection of data (3 coordinates). Vector::length() and Vector::unit() are the only functions, and they don't change anything for they are both const. I prefer using class for more complicated structures that also have private members. Therefore I use struct to stay consistent with my own coding style :)


///
/// vector.h
///
///


#ifndef VECTOR_H
#define VECTOR_H



/// ////// ///
/// Vector ///
/// ////// ///

// A Vector in space
struct Vector
{
public:

double x;
double y;
double z;

Vector operator- () const;
Vector operator* ( const double d ) const;
Vector operator/ ( const double d ) const;
const Vector& operator+= ( const Vector& v );
const Vector& operator-= ( const Vector& v );
const Vector& operator*= ( const double d );
const Vector& operator/= ( const double d );

// Get the unit Vector with length 1
Vector unit() const;

// Get the length of a Vector
double length() const;

};

Vector operator+ ( const Vector& v1, const Vector& v2 );
Vector operator- ( const Vector& v1, const Vector& v2 );

// Return a Vector
Vector newVector( double x, double y, double z );

// Create the cross product between a and b
Vector cross( const Vector& a, const Vector& b );

// Create the dot product between a and b
double dot( const Vector& a, const Vector& b );

// Get the distance between two Vectors
double dist( const Vector& v1, const Vector& v2 );



#endif // VECTOR_H

Share this post


Link to post
Share on other sites
c4c0d3m0n    100
I have one more question about const: Is there any point having my function parameter be const double d instead of double d? Seeing as the function will receive a copy of d instead of a reference, it shouldn't really matter wether it's a const or not right?

Share this post


Link to post
Share on other sites
rip-off    10979
I doesn't matter really. You tend to use const for reference or pointer parameters, not stuff passed by value. On the other hand, it does prevent you accidentally assigning to a parameter, which is usually unintended. It is up to you.

Share this post


Link to post
Share on other sites
visitor    643
This has been pointed out before but your class could use a constructor. As it is, if you write Vector v; in your code, the data members contain garbage. It would also make more sense to turn newVector into another constructor overload:


class Vector
{
public:
//default constructor, initalize members to known values
Vector(): x(0.0), y(0.0), z(0.0) {}

//previous newVector, constructor with parameters
Vector(double x, double y, double z): x(x), y(y), z(z) {}
...
};

Share this post


Link to post
Share on other sites
Zahlman    1682
Implement the "immediate" operators (+=, -= etc.) - the ones you have as member functions - directly, and the binary ones - the ones you have as free functions - in terms of those; not the other way around.


// Notice we don't need 'this' explicitly for most things: variable names within
// a member function are also looked up within 'this' implicitly.
Vector& Vector::operator+=(const Vector& v) {
x += v.x; y += v.y; z =+ v.z;
return *this;
}

// Make an unnamed copy of the first vector, add the second to the copy, and return the copy.
Vector operator+(const Vector& a, const Vector& b) {
return Vector(a) += b;
}


This way is more efficient for the operator+= (instead of making a copy implicitly via operator+ and then assigning it back, you just change the internal data), no more inefficient for operator+ (you have to make a copy either way, to have a definition that makes sense), idiomatic (this kind of thing has been done a zillion times before, and that's the agreed-upon best practice), and arguably much more natural (we do the member version by working with data, and the free function by working with the vector's interface - the usual organization).

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this