Please review my operator overloading

Started by
23 comments, last by Zahlman 16 years ago
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
///

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;
Adamhttp://www.allgamedevelopment.com
// Return a Vectorstatic 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
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. :)
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.
Yeah your right about that, strange he did it that way.
Adamhttp://www.allgamedevelopment.com
Quote:Original post by Porthos
// Return a Vectorstatic 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.
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);}
Adamhttp://www.allgamedevelopment.com
Quote:Original post by Simian Man
Quote:Original post by Porthos
// Return a Vectorstatic 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.
Adamhttp://www.allgamedevelopment.com
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.cpptemp.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;};// freeNum 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;}

This topic is closed to new replies.

Advertisement