C++ Vector2 Class Review

Question

Quite some time ago I started this question: Mathematical Vector2 class implementation and have since improved on it. I've been using it and it seems to work fine, but I'm always looking for ways to improve on code. I also have a question, should I force inline for all my operator overloads or should i just leave them as inline and hope they inline?

Edit 1: Ok, so I don't need inline it looks like. Also I see the benefit of related operations now and have changed my operations and functions to reflect that principle. Did not know that the compiler had better routines for handling assignment operators (though the more I think about it the more it makes sense).

I don't think its a vary good idea to rid my code of const return types, and here is the reason why.

//this operator format
Vector2 Vector2::operator + (Vector2 const & v) const { Vector2 r(*this); return r +=v; }
//allows this
(v1 + v2) = v3
//tested it to be safe

//this operator format
const Vector2 Vector2::operator + (const Vector2 & v) const { Vector2 r(*this); return r += v; }
//does not allow this
(v1 + v2) = v3
//also tested

//this made me realize that, in fact, these are valid as well
Vector2::Add(v1, v2) = v3;
Vector2::Max(v1, v2) = v3;

//But not this
Vector2::Dot(v1, v2) = fval;
//to be on the safe side I changed some more functions return to const
//when I change them to const its true, the qualifer gets thrown out
//but only after the assignment, which is what I want.

Your advice was extremely helpful. Thank you Loki!

Edit 2: Fixed bad operator> code.

fogbit pointed out that I needed to check for division by zero in my Normalize method. But doesn't checking that 'len < FLT_EPSILON' prevent such mishap already?

Second Edit: dsrVector2.hpp

//namespace
namespace dsr
{
//vector2
class Vector2
{

public:
    //members
    float32 x, y;

    //ctor
    Vector2(float32 xVal = 0.0, float32 yVal = 0.0);

    //methods
    float32 LengthSq() const;
    float32 Length() const;
    const Vector2 & Skew();
    const Vector2 & Normalize();

    //assingment operators
    Vector2 & Vector2::operator = (const Vector2 & v) { x = v.x; y = v.y; return *this; }
    Vector2 & Vector2::operator = (const float32 & s) { x = s; y = s; return *this; }
    Vector2 & Vector2::operator - (void) { x = -x; y = -y; return *this; }

    //equality operators
    bool Vector2::operator == (const Vector2 & v) const { return (x == v.x) && (y == v.y); }
    bool Vector2::operator != (const Vector2 & v) const { return !(*this == v); }

    //comparison operators
    bool Vector2::operator < (const Vector2 & v) const { return (x < v.x) || ((x == v.x) && (y < v.y)); }
    bool Vector2::operator > (const Vector2 & v) const { return !(*this < v) && !(*this == v); }

    //vector2 to this operators
    Vector2 & Vector2::operator += (const Vector2 & v) { x += v.x; y += v.y; return *this; }
    Vector2 & Vector2::operator -= (const Vector2 & v) { x -= v.x; y -= v.y; return *this; }
    Vector2 & Vector2::operator *= (const Vector2 & v) { x *= v.x; y *= v.y; return *this; }
    Vector2 & Vector2::operator /= (const Vector2 & v) { x /= v.x; y /= v.y; return *this; }

    //vector2 to vector2 operators
    const Vector2 Vector2::operator + (const Vector2 & v) const { Vector2 r(*this); return r += v; }
    const Vector2 Vector2::operator - (const Vector2 & v) const { Vector2 r(*this); return r -= v; }
    const Vector2 Vector2::operator * (const Vector2 & v) const { Vector2 r(*this); return r *= v; }
    const Vector2 Vector2::operator / (const Vector2 & v) const { Vector2 r(*this); return r /= v; }

    //scaler to this operators
    Vector2 & Vector2::operator += (float32 s) { x += s; y += s; return *this; }
    Vector2 & Vector2::operator -= (float32 s) { x -= s; y -= s; return *this; }
    Vector2 & Vector2::operator *= (float32 s) { x *= s; y *= s; return *this; }
    Vector2 & Vector2::operator /= (float32 s) { x /= s; y /= s; return *this; }

    //scaler to vector2 operators
    const Vector2 Vector2::operator + (float32 s) const { Vector2 r(*this); return r += s; }
    const Vector2 Vector2::operator - (float32 s) const { Vector2 r(*this); return r -= s; }
    const Vector2 Vector2::operator * (float32 s) const { Vector2 r(*this); return r *= s; }
    const Vector2 Vector2::operator / (float32 s) const { Vector2 r(*this); return r /= s; }


    //static arithmetic
    static const Vector2 Add (const Vector2 & v1, const Vector2 & v2) { return v1 + v2; }
    static const Vector2 Sub (const Vector2 & v1, const Vector2 & v2) { return v1 - v2; }
    static const Vector2 Mul (const Vector2 & v1, const Vector2 & v2) { return v1 * v2; }
    static const Vector2 Div (const Vector2 & v1, const Vector2 & v2) { return v1 / v2; }

    //static methods
    static float32 DistanceSq (const Vector2 & v1, const Vector2 & v2)
    { return ((v1.x - v2.x) * (v1.x - v2.x)) + ((v1.y - v2.y) * (v1.y - v2.y)); }

    static float32 Distance (const Vector2 & v1, const Vector2 & v2)
    { return sqrt(DistanceSq(v1, v2)); }

    static float32 Dot (const Vector2 & v1, const Vector2 & v2)
    { return ((v1.x * v2.x) + (v1.y * v2.y)); }

    static const Vector2 Min (const Vector2 & v1, const Vector2 & v2)
    { return Vector2(v1.x < v2.x ? v1.x : v2.x, v1.y < v2.y ? v1.y : v2.y); }

    static const Vector2 Max (const Vector2 & v1, const Vector2 & v2)
    { return Vector2(v1.x > v2.x ? v1.x : v2.x, v1.y > v2.y ? v1.y : v2.y); }

    static const Vector2 Clamp (const Vector2 & v, const Vector2 & min, const Vector2 & max)
    { return Min(Max(v, min), max); /*because lifes too short*/ }

    //friend operators
    friend std::ostream & Vector2::operator << (std::ostream & os, const Vector2 & v) { return os << "{" << v.x << ", " << v.y << "}"; }

}; //end vector2

//prototype
void TestVector2();

}//end namespace

Second Edit: dsrVector2.cpp

//namespace
namespace dsr
{

//vector2 ctor
Vector2::Vector2(float32 xVal, float32 yVal) : x(xVal), y(yVal) { }

//vector2 methods
float32 Vector2::LengthSq() const { return x * x + y * y; }

float32 Vector2::Length() const { return sqrt(LengthSq()); }

const Vector2 & Vector2::Skew() { x -= y; y += x; x -= y; return *this; }

const Vector2 & Vector2::Normalize()
{
    float32 len = Length();
    if(len < FLT_EPSILON)
    {
        x = y = 0;
        return *this;
    }
    else
    {
        float32 invLen = 1.0f / len;
        x *= invLen;
        y *= invLen;
        return *this;
    }
}

//test vector2
void TestVector2()
{
    //using
    using std::cout;
    using std::endl;

    //locals
    Vector2 v1 = Vector2(20, 20);
    Vector2 v2 = Vector2(10, 10);
    Vector2 v3 = Vector2();

    //test
    cout << "-> Vector2 Class Testing" << endl << endl;

    //value
    cout << "-> Testing Vector2 Starting Values" << endl;
    cout << "v1 = " << v1 << endl;
    cout << "v2 = " << v2 << endl;
    cout << "v3 = " << v3 << endl;
    cout << endl;

    //vector2 arithmetic
    cout << "-> Testing Vector2 Arithmetic" << endl;
    cout << "v1 + v2 = " << v1 + v2 << endl;
    cout << "v1 - v2 = " << v1 - v2 << endl;
    cout << "v1 * v2 = " << v1 * v2 << endl;
    cout << "v1 / v2 = " << v1 / v2 << endl;
    cout << endl;

    //vector2 assingment arithmetic
    cout << "-> Testing Vector2 Arithmetic to Assingment" << endl;
    v3 += v1; cout << "v3 += v1; v3 = " << v3 << endl;
    v3 -= v2; cout << "v3 -= v2; v3 = " << v3 << endl;
    v3 *= v1; cout << "v3 *= v1; v3 = " << v3 << endl;
    v3 /= v2; cout << "v3 /= v2; v3 = " << v3 << endl;
    cout << endl;

    //vector2 assingment
    cout << "-> Testing Vector2 Assingment" << endl;
    v3 = v2; cout << "v3 = v2; v3 = " << v3 << "; v2 = " << v2 << endl;
    cout << "-(v3) = " << -v3 << endl;
    v3 = 0; cout << "v3 = 0; v3 = " << v3 << endl;
    cout << endl;

    //vector2 comparison
    cout << "-> Testing Vector2 Comparison" << endl;
    if(!(v1 == v2)) { cout << "v1 == v2 is False" << endl; }
    if(v1 != v2) { cout << "v1 != v2 is True" << endl; }
    if(!(v1 < v2)) { cout << "v1 < v2 is False" << endl; }
    if(v1 > v2) { cout << "v1 > v2 is True" << endl; }
    cout << endl;

    //scaler arithmetic
    cout << "-> Testing Vector2 Scaler Arithmetic" << endl;
    cout << "v1 + 10 = " << v1 + 10 << endl;
    cout << "v1 - 10 = " << v1 - 10 << endl;
    cout << "v2 * 10 = " << v2 * 10 << endl;
    cout << "v2 / 10 = " << v2 / 10 << endl;
    cout << endl;

    //scaler assingment arithmetic
    cout << "-> Testing Vector2 Scaler Arithmetic to Assingment" << endl;
    v3 += 20; cout << "v3 += 20; v3 = " << v3 << endl;
    v3 -= 10; cout << "v3 -= 10; v3 = " << v3 << endl;
    v3 *= 20; cout << "v3 *= 20; v3 = " << v3 << endl;
    v3 /= 10; cout << "v3 /= 10; v3 = " << v3 << endl;
    cout << endl;

    //static arithmetic
    cout << "-> Testing Vector2 Static Arithmetic" << endl;
    cout << "Vector2::Add(v1, v2) = " << Vector2::Add(v1, v2) << endl;
    cout << "Vector2::Sub(v1, v2) = " << Vector2::Sub(v1, v2) << endl;
    cout << "Vector2::Mul(v1, v2) = " << Vector2::Mul(v1, v2) << endl;
    cout << "vector2::Div(v1, v2) = " << Vector2::Div(v1, v2) << endl;
    cout << endl;

    //static member functions
    cout << "-> Testing Vector2 Static Member Functions" << endl;
    cout << "Vector2::Distance(v2, v1) = " << Vector2::Distance(v2, v1) << endl;
    cout << "Vector2::DistanceSq(v2, v1) = " << Vector2::DistanceSq(v2, v1) << endl;
    cout << "Vector2::Dot(v2, v1) = " << Vector2::Dot(v2, v1) << endl;
    cout << "Vector2::Min(v1, v2) = " << Vector2::Min(v1, v2) << endl;
    cout << "Vector2::Max(v1, v2) = " << Vector2::Max(v1, v2) << endl;
    cout << "Vector2::Clamp(Vector2(5, 30), v2, v1) = " << Vector2::Clamp(Vector2(5, 30), v2, v1) << endl;
    cout << endl;

    //member functions
    cout << "-> Testing Vector2 Member Functions" << endl;
    cout << "v1.Length() = " << v1.Length() << endl;
    cout << "v1.LengthSquared() = " << v1.LengthSq() << endl;
    cout << "v2.Skew() = " << v2.Skew() << endl;
    cout << "v2.Normalize() = " << v2.Normalize() << endl;
    cout << endl;

    //testing cpp copy
    cout << "-> Testing Vector2 Default Copy Ctor" << endl;
    Vector2 * vec = new Vector2(v2);
    cout << "v2 = " << v2 << endl;
    cout << "Vector2 * vec = new Vector2(v2);" << endl;
    cout << "v4 = " << *vec << endl;
    cout << endl;

    //free
    delete vec;
}

}//end namespace

BTW, I know this is not the same account. I lost my info for that account and have since been using this account.

Answer 1

float32 Vector2::Length() const { return sqrt(x * x + y * y); }
float32 Vector2::LengthSq() const { return x * x + y * y; }

Don't dup the code. You already have the functionality for calculating LenSq use it instead of writing it again.

float32 Vector2::Length() const { return sqrt( LengthSq() ); }
float32 Vector2::LengthSq() const { return x * x + y * y; }

upd:

Vector2 & Vector2::Normalize()
{
    float32 invLen = 1.0f / len;
}

what if len is 0?

upd2:

General recommendation, use += instead of +. It's shorter and in this case the new object isn't created. If you have deal with native types (ints etc), then creation of a new object isn't a problem, but the user defined objects might be very big. Compare strings below.

Vector2 & Vector2::Skew() { x = x - y; y = x + y; x = x - y; return *this; }
Vector2 & Vector2::Skew() { x -= y; y += x; x -= y; return *this; }

upd3: Don't use endl so often, i don't think you need this. Check Google/Stackoverflow for the question "\n vs endl"

Answer 2

Don't delcare members inline:

inline Vector2 & Vector2::operator = (const Vector2 & v) { x = v.x; y = v.y; return *this; }

It is already inline. This provides no information for humans (and the compiler ignores this keyword) so best not to use this keyword except in places where you must. https://stackoverflow.com/q/1759300/14065

Define related operations in terms of each other.
This prevents subtle bugs:

inline bool Vector2::operator != (const Vector2 & v) const { return (x != v.x) || (y != v.y); }

// Much better to write as:

bool Vector2::operator != (Vector2 const& v) const { return !(*this == v);}
//or
bool Vector2::operator != (Vector2 const& v) const { return !this->operator==(v);}

Define the standard operators in terms of their assignment versions.
Again this helps in preventing simple mistakes by reducing repeated code.
This case is also widely used and compilers have routines to optimize this.

inline const Vector2 Vector2::operator + (const Vector2 & v) const { return Vector2(x + v.x, y + v.y); }

// Much better to write as

Vector2 Vector2::operator + (Vector2 const& v) const { Vector2 r(*this); return r += v;}

Also Note: Returning by const value is meaningless.

inline const Vector2 Vector2::operator +(/*STUFF*/)
//     ^^^^^^^^^^^^^

Its a value once at the call site it is a temporary value thus its const is removed.

Again with the static version of standard operators. Define them in terms of normal operators. Prevent copy and paste bugs.

static Vector2 Add (const Vector2 & v1, const Vector2 & v2) { return Vector2(v1.x + v2.x, v1.y + v2.y); }

// Much better as:

static Vector2 Add (Vector2 const& v1, Vector2 const& v2) { return v1 + v2; }

Define related operators in terms of each other.
Reduce repeated code and subtle bugs:

static float32 Distance (const Vector2 & v1, const Vector2 & v2)
{ return sqrt(DistanceSq(v1,v2)); }

If you define the output operator also define the input operator (that can read what was generated by output). Also there is no need to add an extra statement to return os;. Just return the expression.

friend std::ostream & Vector2::operator << (std::ostream & os, const Vector2 & v)
{ 
    return os << "{" << v.x << ", " << v.y << "}";
}
friend std::istream & Vector2::operator >> (std::istream & is, Vector2 & v)
{
    char tmp1=' ';
    char tmp2=' ';
    char tmp3=' ';
    is >> tmp >> v.x >> tmp2 >> v.y >> tmp3;

    if ((tmp1 != '{' ) || (tmp2 != ',') || (tmp3 != '}'))
    {   is.setstate(std::ios::failbit);
    }

    return os;
}

Answer 3

Most binary operators are best defined outside the class in order to make them more symmetric. I would recommend using the following pattern:

Vector2 const operator+(Vector2 lhs, Vector2 const& rhs) {
    return lhs += rhs;
}

Idem for all similar operators. Feel free to make the return type const if you'd like, although that's not always desired ((v1 + v2).Normalize() may be useful).

Most of your static functions look like they should be free functions to me. I don't see a use-case for Add.