I'd like a critique of my mathematical vector struct

[Armendhammer]

I wrote a struct with operator overloading as practice, I want to know what I've done wrong and how I can make it better.
Edit: would this thread be better suited in the beginners section?

struct vec3d
{
	double x, y, z;
	//vec3d() : x(), y(), z() {}
	//vec3d(double x, double y, double z) : x(x), y(y), z(z) {}
	vec3d(double x, double y, double z) { this->x = x; this->y = y; this->z = z; }
	vec3d() {};
	vec3d& operator = (const vec3d &rhs);
	bool operator == (const vec3d &rhs) const;
	bool operator != (const vec3d &rhs) const;
        //from a maths standpoint, I'm not sure how much sense the operators below make
	bool operator > (const vec3d &rhs) const;
	bool operator < (const vec3d &rhs) const;
	bool operator >= (const vec3d &rhs) const;
	bool operator <= (const vec3d &rhs) const;
};

vec3d& vec3d::operator = (const vec3d &v)
{
	if (!(this == &v)) x = v.x, y = v.y, z = v.z;
	return *this;
}
//vector-vector ops begin
bool vec3d::operator == (const vec3d &rhs) const
{
	return x == rhs.x && y == rhs.y && z == rhs.z;
}
bool vec3d::operator != (const vec3d &v) const
{
	return !(*this == v);
}
bool vec3d::operator > (const vec3d &rhs) const
{
	return this->x > rhs.x && this->y > rhs.y && this->z > rhs.z;
}
bool vec3d::operator < (const vec3d &rhs) const
{
	return this->x < rhs.x && this->y < rhs.y && this->z < rhs.z;
}
bool vec3d::operator >= (const vec3d &rhs) const
{
	return this->x >= rhs.x && this->y >= rhs.y && this->z >= rhs.z;
}
bool vec3d::operator <= (const vec3d &rhs) const
{
	return this->x >= rhs.x && this->y >= rhs.y && this->z >= rhs.z;
}

vec3d operator -(const vec3d &a) //unary negation, avoid confusion
{
	return vec3d(-a.x, -a.y, -a.z);
}
	
vec3d operator + (const vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs.x + rhs.x, lhs.y + rhs.y, lhs.z + rhs.z);
}
vec3d operator - (const vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs + -rhs);
}
vec3d operator * (const vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs.x * rhs.x, lhs.y * rhs.y, lhs.z * rhs.z);
}
vec3d operator / (const vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs.x / rhs.x, lhs.y / rhs.y, lhs.z / rhs.z);
}

vec3d operator += (vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs = lhs + rhs);
}
vec3d operator -= (vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs = lhs - rhs);
}
vec3d operator *= (vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs = (lhs * rhs));
}
vec3d operator /= (vec3d &lhs, const vec3d &rhs)
{
	return vec3d(lhs = lhs / rhs);
}
//vector-scalar ops begin
vec3d operator * (double lhs, const vec3d &rhs)
{
	return vec3d(lhs * rhs.x, lhs * rhs.y, lhs * rhs.z);
}
vec3d operator * (const vec3d &lhs, double rhs)
{
	return vec3d(lhs.x * rhs, lhs.y * rhs, lhs.z * rhs);
}
vec3d operator / (const vec3d &lhs, double rhs)
{
	return vec3d((1 / rhs) * lhs);
}
vec3d operator *= (vec3d &lhs, double rhs)
{
	return vec3d(lhs = rhs * lhs);
}
vec3d operator /= (vec3d &lhs, double rhs)
{
	return vec3d(lhs = lhs / rhs);
}
//vector-scalar ops end
double length(vec3d &v)
{
	return sqrt(v.x * v.x + v.y * v.y + v.z * v.z);
}
double dot(vec3d &a, vec3d &b)
{
	return a.x * b.x + a.y * b.y + a.z * b.z;
}
double radian_angle_between(vec3d &a, vec3d &b)
{
	double theta = dot(a, b) / (length(a) * (length(b)));
	return acos(theta);
}
vec3d floor_vec3d(vec3d &a)
{
	return vec3d(floor(a.x), floor(a.y), floor(a.z));
}
bool almost_equal(vec3d &a, vec3d &b, double epsilon)
{
	if (floor(a.x - b.x) <= epsilon && floor(a.y - b.y) <= epsilon && floor(a.z - b.z) <= epsilon)
	{
		return true;
	}
	else return false;
}
bool orthogonal(vec3d &a, vec3d &b)
{
	return dot(a, b) == 0;
}
bool orthogonal_almost(vec3d &a, vec3d &b, double epsilon)
{
	return floor(dot(a, b)) <= epsilon;
}
vec3d cross(vec3d &a, vec3d &b)
{
	 return vec3d(a.y * b.z - a.z * b.y, a.z * b.x - a.x * b.z, a.x * b.y - a.y * b.x);
}
vec3d normalize(vec3d &a)
{
	return a / length(a);
}
void print(vec3d &a)
{
	std::cout << a.x << ", " << a.y << ", " << a.z << "\n";
}

[ne555]

> vec3d& operator = (const vec3d &rhs);
¿what's wrong with the one provided by the compiler?

> from a maths standpoint, I'm not sure how much sense the operators below make
¿then why did you make them?

> return vec3d(lhs = lhs - rhs);
¿why do you need the `vec3d' there?

vec3d operator * (const vec3d &lhs, double rhs)
{
	//return vec3d(lhs.x * rhs, lhs.y * rhs, lhs.z * rhs);
	return rhs*lhs;
}

> double length(vec3d &v)
¿why a non-const reference?
you cannot pass a temporary this way.


Also, ¿is this all in a header?

[JLBorges]

If you're writing a function that can be implemented as either a member or as a non-friend non-member, you should prefer to implement it as a non-member function. That decision increases class encapsulation. When you think encapsulation, you should think non-member functions. - Scott Meyers in Dr. Dobb's

http://www.drdobbs.com/cpp/how-non-member-functions-improve-encapsu/184401197?pgno=1

Canonical implementations of binary arithmetic operators:

Since for every binary arithmetic operator there exists a corresponding compound assignment operator, canonical forms of binary operators are implemented in terms of their compound assignments

'Canonical implementations' in http://en.cppreference.com/w/cpp/language/operators

#include <iostream>
#include <tuple>

struct vec3d
{
	explicit vec3d( double x = 0, double y = 0, double z= 0 ) : x(x), y(y), z(z) {}

	std::tuple<double,double,double> tuple() const { return std::make_tuple( x, y, z ) ; }

	vec3d& operator += ( const vec3d& that ) { x += that.x ; y += that.y ; z += that.z ;  return *this ; }
	// like-wise for other compound assignment operators

	private: double x, y, z;

	friend std::ostream& operator<< ( std::ostream& stm, const vec3d& vec )
        { return stm << "vec3d { " << vec.x << ", " << vec.y << ", " << vec.z << " }" ; }

};

bool operator== ( const vec3d& a, const vec3d& b ) { return a.tuple() == b.tuple() ; }
bool operator< ( const vec3d& a, const vec3d& b ) { return a.tuple() < b.tuple() ; }
// like-wise for other comparison operators

vec3d operator+ ( vec3d a, const vec3d& b ) { return a += b ; } // pass lhs by value, reuse compound assignment
// like-wise for other binary arithmetic operators

int main()
{
    std::cout << vec3d( 1, 2, 3 ) + vec3d( 4, 5, 6 ) << '\n' ;
}

http://coliru.stacked-crooked.com/a/001a8cb6ae8524f0

[dhayden]

Line 20: This works but it's much more common to do :

if (!(this == &v)) {
    x = v.x;
    y = v.y;
    z = v.z;
}

Operator*() and dot() do the same thing. One should be implemented in terms of the other.

All the functions from line 109-150 should be methods.

It's best to define +,-,*,/ in terms of +=, -=, *= and /=. For example:

vec3d operator+(vec3d left, const vec3d &right) {
    return left += right;
}

Change print() to a global << operator that can use any ostream:

ostream &operator <<(ostream &os const vec3d &v) {
    return os << v.x << ", " << v.y << ", " << v.z;
}

[Armendhammer]

@ne555, this code is not in a header.
@dhayden, dot and *(vec3d, vec3d) do not do the same thing, one returns a scalar and the other returns a vector respectively.

Dhayden says that functions from 109-150 should be methods, but JLBorges disagrees. I agree with JLBorges.

I've added declarations to keep the overloads organized.

namespace math3d 
{
	struct vec3d
	{
		double x, y, z;
		explicit vec3d(double x = 0, double y = 0, double z = 0) : x(x), y(y), z(z) {};
		bool operator == (const vec3d &rhs) const;
		bool operator != (const vec3d &rhs) const;
                friend std::ostream& operator << (std::ostream& ostrm, const vec3d& a)
		{
			return ostrm << "vec3d (" << a.x << ", " << a.y << ", " << a.z << ")" << "\n";
		}
	};

	vec3d operator + (const vec3d &lhs, const vec3d &rhs);
	vec3d operator - (const vec3d &lhs, const vec3d &rhs);
	vec3d operator - (const vec3d &a);
	vec3d operator * (double lhs, const vec3d &rhs);
        vec3d operator * (const vec3d &lhs, const vec3d &rhs);
	vec3d operator / (const vec3d &lhs, const vec3d &rhs);
	vec3d operator += (const vec3d &lhs, const vec3d &rhs);
	vec3d operator -= (const vec3d &lhs, const vec3d &rhs);
	vec3d operator *= (const vec3d &lhs, const vec3d &rhs);
	vec3d operator /= (const vec3d &lhs, const vec3d &rhs);

	//vector-vector ops begin
	bool vec3d::operator == (const vec3d &rhs) const
	{
		return x == rhs.x && y == rhs.y && z == rhs.z;
	}
	bool vec3d::operator != (const vec3d &a) const
	{
		return !(*this == a);
	}

	vec3d operator + (const vec3d &lhs, const vec3d &rhs)
	{
		return vec3d(lhs.x + rhs.x, lhs.y + rhs.y, lhs.z + rhs.z);
	}
	vec3d operator - (const vec3d &lhs, const vec3d &rhs)
	{
		return lhs + -rhs;
	}
	vec3d operator * (const vec3d &lhs, const vec3d &rhs)
	{
		return vec3d(lhs.x * rhs.x, lhs.y * rhs.y, lhs.z * rhs.z);
	}
	vec3d operator / (const vec3d &lhs, const vec3d &rhs)
	{
		return vec3d(lhs.x / rhs.x, lhs.y / rhs.y, lhs.z / rhs.z);
	}

	vec3d operator += (vec3d &lhs, const vec3d &rhs)
	{
		return lhs = lhs + rhs;
	}
	vec3d operator -= (vec3d &lhs, const vec3d &rhs)
	{
		return lhs = lhs - rhs;
	}
	vec3d operator *= (vec3d &lhs, const vec3d &rhs)
	{
		return lhs = lhs * rhs;
	}
	vec3d operator /= (vec3d &lhs, const vec3d &rhs)
	{
		return lhs = lhs / rhs;
	}
	//vector-scalar ops begin
	vec3d operator - (const vec3d &a) //unary negation, avoid confusion
	{
		return -1.0 * a;
	}
	vec3d operator * (double lhs, const vec3d &rhs)
	{
		return vec3d(lhs * rhs.x, lhs * rhs.y, lhs * rhs.z);
	}
	vec3d operator * (const vec3d &lhs, double rhs)
	{
		return vec3d(lhs.x * rhs, lhs.y * rhs, lhs.z * rhs);
	}
	vec3d operator / (const vec3d &lhs, double rhs)
	{
		return (1 / rhs) * lhs;
	}
	vec3d operator *= (vec3d &lhs, double rhs)
	{
		return lhs = rhs * lhs;
	}
	vec3d operator /= (vec3d &lhs, double rhs)
	{
		return lhs = lhs / rhs;
	}
	//vector-scalar ops end
	double length(const vec3d &a)
	{
		return sqrt(a.x * a.x + a.y * a.y + a.z * a.z);
	}
	double dot(const vec3d &a, const vec3d &b)
	{
		return a.x * b.x + a.y * b.y + a.z * b.z;
	}
	double radian_angle_between(const vec3d &a, const vec3d &b)
	{
		double theta = dot(a, b) / (length(a) * (length(b)));
		return acos(theta);
	}
	vec3d floor_vec3d(const vec3d &a)
	{
		return vec3d(floor(a.x), floor(a.y), floor(a.z));
	}
	bool almost_equal(const vec3d &a, const vec3d &b, double epsilon)
	{
		if (floor(a.x - b.x) <= epsilon && floor(a.y - b.y) <= epsilon && floor(a.z - b.z) <= epsilon)
		{
			return true;
		}
		else return false;
	}
	bool orthogonal(const vec3d &a, const vec3d &b)
	{
		return dot(a, b) == 0;
	}
	bool orthogonal_almost(const vec3d &a, const vec3d &b, double epsilon)
	{
		return floor(dot(a, b)) <= epsilon;
	}
	vec3d cross(const vec3d &a, const vec3d &b)
	{
		 return vec3d(a.y * b.z - a.z * b.y, a.z * b.x - a.x * b.z, a.x * b.y - a.y * b.x);
	}
	vec3d normalize(const vec3d &a)
	{
		return a / length(a);
	}
}

[TheIdeasMan]

hi,

With radian_angle_between, are you aware that the dot product equals the cosine of the angle between the 2 vectors?

With orthogonal, there are issues with that regarding the equality with floating point numbers, also with operator==. You have orthogonal_almost which on the face of it might be better, as it attempts to address those issues. But it has a floor function, how will that work if the value is say -3e-15, giving -1.0? Should you be using the std::abs function rather than std::floor in that equation, and elsewhere for the floor function? Similarly, I wonder aboutf the floor_vec3d .

Perhaps pedantically, the parameter epsilon might be a little confusing: Is it really the std::numeric_limits::epsilon or is it more of a precision value? To me, the latter might be of more use.

This is the first time I have seen anyone implement a "vector division" operation (as opposed to scalar division). What is the purpose of it? I did vector algebra at university, I don't recall any of the formulae having "vector division" like that.

With normalize I have seen that name used a lot, but I am still mystified why it is called that, as opposed to the concept of a unit vector (length 1.0). What does one name a unit vector which is normal to a plane? A normalized normal vector?