Jump to content
  • Advertisement
Sign in to follow this  
c4c0d3m0n

Please review my operator overloading

This topic is 3687 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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
Advertisement
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

// 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
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
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
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
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
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
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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!