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 on other sites
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 on other sites
// 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

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 on other sites
Quote:
 Original post by adam23For 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 on other sites
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 PorthosVector 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 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 on other sites
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 PorthosVector 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 on other sites
Quote:

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;}

Share on other sites
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 on other sites
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 spacestruct 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 VectorVector newVector( double x, double y, double z );// Create the cross product between a and bVector crossproduct( Vector a, Vector b );// Get the distance between two Vectorsdouble dist( Vector v1, Vector v2 );#endif // VECTOR_H

Share on other sites
Quote:
 Original post by c4c0d3m0nI'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 on other sites
This may help with your understanding of the use of const.
http://www.possibility.com/Cpp/const.html

also if you use google with "c++ const use" should provide a lot more atricles on it's use.

Share on other sites
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)).

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 on other sites
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 on other sites
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 spacestruct 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 VectorVector newVector( double x, double y, double z );// Create the cross product between a and bVector crossproduct( Vector a, Vector b );// Get the distance between two Vectorsdouble 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 on other sites
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 on other sites
Quote:
 Original post by Simian ManAlso 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 on other sites
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 on other sites
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 spacestruct 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 VectorVector newVector( double x, double y, double z );// Create the cross product between a and bVector cross( const Vector& a, const Vector& b );// Create the dot product between a and bdouble dot( const Vector& a, const Vector& b );// Get the distance between two Vectorsdouble dist( const Vector& v1, const Vector& v2 );#endif // VECTOR_H

Share on other sites
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 on other sites
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 on other sites
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 on other sites
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).

Create an account

Register a new account

• Forum Statistics

• Total Topics
628306
• Total Posts
2981944

• 10
• 11
• 11
• 11
• 10