Hi there! I need someone who knows quite a bit about the OOP aspect of C++ and would be kind enough to take a look at my VECTOR class to see if I have implemented it correctly. By now I have been doing mixed C/C++ and I don''t know if what I''ve written in C++ is efficient or not (I know that I need to inline most of my member functions). What I''m looking for is whether or not I have to include the operators overloading in the class or not, also if I have to return a VECTOR& or VECTOR from a function (like the Cross Product), basically that kind of stuff. Anyone willing to help me? What if I say "Pleaaaaasssee?" Thank you people! You''re the best! ---------------------------------------------- That's just my 200 bucks' worth!
..-=gLaDiAtOr=-..

Here is the source code!

    /////////////////// VECTOR.H ///////////////////////#ifndef __VECTOR_H#define __VECTOR_Hclass VECTOR{public:	VECTOR();	VECTOR( float _x, float _y, float _z );	VECTOR( VECTOR &v );	~VECTOR();	float	Magnitude();	float	Dot( VECTOR &v );	VECTOR	Cross( VECTOR &v );	void	Normalize();	void	SetX( float _x );	void	SetY( float _y );	void	SetZ( float _z );	void	SetVector( VECTOR &v );	void	SetVector( float _x, float _y, float _z );	float	GetX();	float	GetY();	float	GetZ();	VECTOR	GetVector();	int	operator == ( VECTOR &v );	VECTOR operator =  ( VECTOR &v );	VECTOR operator +  ( VECTOR &v );	VECTOR operator -  ( VECTOR &v );	VECTOR operator *  ( VECTOR &v );	VECTOR operator /  ( VECTOR &v );protected:	float x, y, z;};#endif////////////////////////// VECTOR.H END ////////////////////////////////////////////////////////////////////////////////////////////////////////////////// VECTOR.CPP //////////////////////////#include <math.h>#include "vector.h"VECTOR::VECTOR(){	x = 0.0;	y = 0.0;	z = 0.0;}VECTOR::VECTOR( float _x, float _y, float _z ){	x = _x;	y = _y;	z = _z;}VECTOR::VECTOR( VECTOR &v ){  SetVector(v.x, v.y, v.z);}VECTOR::~VECTOR(){}float VECTOR::Magnitude(){	return (float) sqrt( x*x + y*y + z*z );}float VECTOR::Dot( VECTOR &v ){	return (x*x + y*y + z*z);}VECTOR VECTOR::Cross( VECTOR &v ){	return VECTOR(x*v.z-z*v.y, z*v.x-x*v.z, x*v.y-y*v.x);}void VECTOR::Normalize(){	float	length = 1/Magnitude();	x *= length;	y *= length;	z *= length;	}void VECTOR::SetX( float _x ){	x = _x;}void VECTOR::SetY( float _y ){	y = _y;}void VECTOR::SetZ( float _z ){	z = _z;}void VECTOR::SetVector( VECTOR &v ){	SetVector(v.x, v.y, v.z);}void VECTOR::SetVector( float _x, float _y, float _z){	x = _x;	y = _y;	z = _z;}float VECTOR::GetX(){	return x;}float VECTOR::GetY(){	return y;}float VECTOR::GetZ(){	return z;}VECTOR VECTOR::GetVector(){	return *this;}VECTOR VECTOR::operator =  ( VECTOR &v ){	SetVector( v );	return *this;}int VECTOR::operator == ( VECTOR &v ){	// FIX: Normalize both vectors first!! SLOW!!!!	return (x == v.x && y == v.y && z == v.z);}VECTOR VECTOR::operator +  ( VECTOR &v ){	return VECTOR(x+v.x, y+v.y, z+v.z);}VECTOR VECTOR::operator -  ( VECTOR &v ){	return VECTOR(x-v.x, y-v.y, z-v.z);}VECTOR VECTOR::operator *  ( VECTOR &v ){	return VECTOR(x*v.x, y*v.y, z*v.z);}VECTOR VECTOR::operator /  ( VECTOR &v ){	return VECTOR(x/v.x, y/v.y, z/v.z);}////////////////////////// VECTOR.CPP END ///////////////////////    

----------------------------------------------
That's just my 200 bucks' worth!

..-=gLaDiAtOr=-..

quote:
Until the other day there were so many flames I could hardly survive... Now, when someone needs help, there''s no one that can do anything about it?

Give us a chance

If you are only going to assign vectors to other vectors, it''s ok to leave the operators in the class. The reason for putting them outside is that you can have a none-type paramater on the left hand side. For example in a string class, if you only had the member operator+ this would work

str + "hello";

but this would not:

"hello" + str;

to fix it you would need both of these

MyString& operator(const char* psz, const MyString& str);
MyString& operator(const MyString& str, const char* psz);

So your vectors operators are fine.

[operator]
also if I have to return a VECTOR& or VECTOR from a function (like the Cross Product),
[/operator]

Never return a reference to a local variable. A reference to a member or to ''this'' is ok, but local variables get destroyed when the function returns. You''ll get all kinds of problems if you try to return a reference to a local variable.

You might be tempted to return a reference to a static to try and make it more efficient, but that will give you problems as well. e.g.

foo(vec1.Dot(), vec2.Dot());

here Dot will be called for both vectors before foo is called, and the references returned by Dot are to the same static. so foo isn''t going to work the way you want.

If you inline your functions the compiler will probably give you code just as fast as if you did return a reference (maybe faster) but without any of the problems.

But your returning by value so thats ok

quote:
#define __VECTOR_H
...
VECTOR( float _x, float _y, float _z );

Any names starting with _ or __ are reserved for the standard and the implementation (e.g. MS or Borland). That''s why things like __int64 are called what they are. You shouldn''t start names with _ or __ (or even ___, etc. just in case). Starting member vars with m_ or ending them with _ is the most common (safe) way to do it. Then leave all local and parameter variables without the _.

quote:
~VECTOR();

Your destructor isn''t virtual, which means you shouldn''t use it as a base class (the derived classes wont be destroyed properly if you delete them through a pointer). So your protected data (x, y, z) should be private, or make your destructor virtual. Add a virtual function with add a vtable (a lookup table to see where the virtual functions really are) to your class which can add to its size, and since this class is pretty small and will be used a lot you''ll probably be better making your data private.

quote:
int operator == ( VECTOR &v );

If your not going to change the parameter (which you shouldn''t in op==), it''s better to pass by const& (i.e. operator == ( const VECTOR &v ) so that you can call it with const vectors. Also any function that doesn''t change vector should be a const function, so you can call it on const vectors. e.g.

float GetX();

should be

float GetX() const;

Only const functions can be called on const objects, and inside a const function ''this'' is const. So all your member variables are also const (i.e. read-only).

If you don''t have any const functions, a const vector will be of no use - and there will be times when you want one. When you start using const, make sure you use it everywhere! const member functions can''t call none-const member functions. So if you don''t use cost properly it could lead to lots of problems.

Also operator == should probably return ''bool'' instead of ''int''

quote:
VECTOR::VECTOR( float _x, float _y, float _z )
{
x = _x;
y = _y;
z = _z;
}

In this constructor your default constructing three floats, and then assigning values to them. It''s kinda like doing this:

float x;
x = _x;
float y;
y = _y;
float z;
z = _z;

It would be more efficient (but only by a tiny amount with floats, if at all - it does make difference with more complicated types) if you assigned then when you create them. The same as you would do:

float x = _x;
float y = _y;
float z = _z;

To do this you need to use an initialiser list. An initialiser list allows to pass parameters to the constructors of member variables are base classes. All the built in types have constructors that take an initial value. So for your vector class, you would do this:

VECTOR::VECTOR( float _x, float _y, float _z ) : x(_x), y(_y), z(_z){} 

The : after the ctor name starts an initialiser list, and you separator each member with a comma. This is the same as:

float x(_x);
float y(_y);
float z(_z);

quote:
VECTOR VECTOR::GetVector()
{
return *this;
}

I don''t see the point in this function. ''vec.GetVector()'' is exactly the same as ''vec'' (apart from the temporary vector that gets crated from the return by value). If I where I''d kill this function.

- Wilka

quote: Give us a chance

Sure

Anyways... That''s the EXACT reply I wanted to hear. You''ve been great. If you can add a few more tips like you did just now, that''d be superb. And do you have any recommendations of a good website where I can get some tips when working with C++ similar to the ones you described? Thanks a bunch!!!

----------------------------------------------
That's just my 200 bucks' worth!

..-=gLaDiAtOr=-..

Good reply.

I''d add that you really want to return a VECTOR& with operator =, so you can chain assignments:
a = b = c = d;

Also, if you''re going to be doing a LOT of vector math, you might want to overload +=, -=, *=, /=, etc. These functions can be faster than the default implementation.

For instance, without a specific += operator overload, the following statement:
a += b;
translates to
a = a + b;
...which translates to
a.operator = (a.operator+ (b))
...which has two function calls and a temporary VECTOR (returned by a.opererator + (b)). However, if you overload operator +=, you get one function call & no temporary:
VECTOR& operator += (const VECTOR& rhs)
{
x += rhs.x;
y += rhs.y;
z += rhs.z;
return *this;
}

Small performance gain, but it might matter in the long run.

Remember, these operators should return a VECTOR:
+ - / *
..and these operators should return a VECTOR&:
= -= += /= *=

For a really good discussion, check out Scott Meyer''s book "Effective C++". He goes into great depth about what makes a good, efficient class. He taught me most of what I just wrote here.

Make x,y,z public and you can get rid of the following functions:

void SetX( float _x );
void SetY( float _y );
void SetZ( float _z );
float GetX();
float GetY();
float GetZ();

I wouldn''t make the x,y,z private. It is better to be safe then sorry and you don''t want any other classes changing the values unless they go through the functions. That is just my 2 cents.

WOW!! People, you''re just GREAT!! Keep those tips & tricks coming ... It''s gonna be one hell of a class... (both meanings of the word )

----------------------------------------------
That's just my 200 bucks' worth!

..-=gLaDiAtOr=-..

This isn't actually part of the class, but I think it is a good idea to add functions that work on the class data. For instance, make a function, Vector Normalize(const Vector &v). That way instead of doing :

Vector v;
v = some_vector;
v.Normalize();

You could just do :

Normalize(some_vector)). That is often very usefull if you don't want to modify a vector, but still want it normalized for another puprose. And you can do it for other functions like the dot and cross product.

Edited by - zerwit on July 21, 2000 8:53:10 PM

Thank you very much everybody! That''s all I wanted to hear. Now I can safely implement what I think would work for me (and most of them fall into this category ). Once again thanks!

----------------------------------------------
That's just my 200 bucks' worth!

..-=gLaDiAtOr=-..