Review of 2d Vector class

Question

I'll keep this short. I've never actually done professional C++. I don't really know any of the 'best practices'. I'd like to get some review on a simple class that I've made.

My Vector2d.h file:

#ifndef VECTOR2D_H
#define VECTOR2D_H

#include <cfloat>
#include <climits>

/*The Vector2d class is an object consisting of simply an x and
  y value. Certain operators are overloaded to make it easier
  for vector math to be performed.*/

class Vector2d {
public:
    /*The x and y values are public to give easier access for
      outside funtions. Accessors and mutators are not really
      necessary*/
    float x;
    float y;

    //Constructor assigns the inputs to x and y.
    Vector2d();
    Vector2d(float, float);

    /*The following operators simply return Vector2ds that
      have operations performed on the relative (x, y) values*/
    Vector2d operator+(const Vector2d&) const;
    Vector2d operator-(const Vector2d&) const;
    Vector2d operator*(const Vector2d&) const;
    Vector2d operator/(const Vector2d&) const;

    //Check if the Vectors have the same values.
    bool operator==(const Vector2d&) const;

    /*Check which Vectors are closer or further from the
      origin.*/
    bool operator>(const Vector2d&) const;
    bool operator<(const Vector2d&) const;
    bool operator>=(const Vector2d&) const;
    bool operator<=(const Vector2d&) const;

    //Negate both the x and y values.
    Vector2d operator-() const;

    //Apply scalar operations.
    Vector2d operator*(const float&) const;
    Vector2d operator/(const float&) const;

    //Product functions
    static float DotProduct(const Vector2d&, const Vector2d&);
    static float CrossProduct(const Vector2d&, const Vector2d&);

    //Returns the length of the vector from the origin.
    static float Magnitude(const Vector2d&);

    //Return the unit vector of the input
    static Vector2d Normal(const Vector2d&);

    //Return a vector perpendicular to the left.
    static Vector2d Perpendicular(const Vector2d&);

    //Return true if two line segments intersect.
    static bool Intersect(const Vector2d&, const Vector2d&, const Vector2d&, const Vector2d&);

    //Return the point where two lines intersect.
    static Vector2d GetIntersect(const Vector2d&, const Vector2d&, const Vector2d&, const Vector2d&);
};

#endif

And for my Vector2d.cpp file:

#include "Vector2d.h"
#include <cmath>

Vector2d::Vector2d()
{
    x = 0.0;
    y = 0.0;
}

Vector2d::Vector2d(float sourceX, float sourceY)
{
    x = sourceX;
    y = sourceY;
}

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

Vector2d Vector2d::operator-(const Vector2d &v) const
{
    return Vector2d(x-v.x, y-v.y);
}

Vector2d Vector2d::operator*(const Vector2d &v) const
{
    return Vector2d(x*v.x, y*v.y);
}

Vector2d Vector2d::operator/(const Vector2d &v) const
{
    return Vector2d(x/v.x, y/v.y);
}

bool Vector2d::operator==(const Vector2d &v) const
{
    return ((x == v.x) && (y == v.y));
}

bool Vector2d::operator>(const Vector2d &v) const
{
    return (x*x + y*y) > (v.x*v.x + v.y*v.y);
}

bool Vector2d::operator<(const Vector2d &v) const
{
    return (x*x + y*y) < (v.x*v.x + v.y*v.y);
}

bool Vector2d::operator>=(const Vector2d &v) const
{
    return (x*x + y*y) > (v.x*v.x + v.y*v.y) ||
           (x*x + y*y) == (v.x*v.x + v.y*v.y);
}

bool Vector2d::operator<=(const Vector2d &v) const
{
    return (x*x + y*y) < (v.x*v.x + v.y*v.y) ||
           (x*x + y*y) == (v.x*v.x + v.y*v.y);
}
Vector2d Vector2d::operator-() const
{
    return Vector2d(-x, -y);
}

Vector2d Vector2d::operator*(const float& scalar) const
{
    return Vector2d(x*scalar, y*scalar);
}

Vector2d Vector2d::operator/(const float& scalar) const
{
    return Vector2d(x/scalar, y/scalar);
}

float Vector2d::DotProduct(const Vector2d &a, const Vector2d &b)
{
    return ((a.x * b.x) + (a.y * b.y));
}

float Vector2d::CrossProduct(const Vector2d &a, const Vector2d &b)
{
    return ((a.x * b.y) - (a.y * b.x));
}

float Vector2d::Magnitude(const Vector2d &v)
{
    return sqrt((v.x * v.x) + (v.y * v.y));
}

Vector2d Vector2d::Normal(const Vector2d &v)
{
    float magnitude = Magnitude(v);
    return Vector2d(v.x / magnitude, v.y / magnitude);
}

Vector2d Vector2d::Perpendicular(const Vector2d &v)
{
    return Vector2d(v.y, -v.x);
}

bool Vector2d::Intersect(const Vector2d &aa, const Vector2d &ab, const Vector2d &ba, const Vector2d &bb)
{
    Vector2d p = aa;
    Vector2d r = ab - aa;
    Vector2d q = ba;
    Vector2d s = bb - ba;

    float t = CrossProduct((q - p), s) / CrossProduct(r, s);
    float u = CrossProduct((q - p), r) / CrossProduct(r, s);

    return (0.0 <= t && t <= 1.0) &&
           (0.0 <= u && u <= 1.0);
}

Vector2d Vector2d::GetIntersect(const Vector2d &aa, const Vector2d &ab, const Vector2d &ba, const Vector2d &bb)
{
    float pX = (aa.x*ab.y - aa.y*ab.x)*(ba.x - bb.x) -
               (ba.x*bb.y - ba.y*bb.x)*(aa.x - ab.x);
    float pY = (aa.x*ab.y - aa.y*ab.x)*(ba.y - bb.y) -
               (ba.x*bb.y - ba.y*bb.x)*(aa.y - ab.y);
    float denominator = (aa.x - ab.x)*(ba.y - bb.y) -
                        (aa.y - ab.y)*(ba.x - bb.x);

    return Vector2d(pX / denominator, pY / denominator);
}

Thanks in advance!

Answer 1

Disclaimer

I will mostly not critique what your class is doing, but mainly suggest how to make your interface as natural as possible (do as the ints do) by operator overloading best practices in terms of return type and const-correctness. I will mostly stick to C++98/03 and only sparingly suggest C++11 features. You could read the revamped GotW series for more information on the C++11 issues (such as uniform initialization and move semantics).

Class definition

#ifndef VECTOR2D_H
#define VECTOR2D_H

#include <cfloat>
#include <climits>

/*The Vector2d class is an object consisting of simply an x and
  y value. Certain operators are overloaded to make it easier
  for vector math to be performed.*/

First, I would change Vector2d into a class template taking a single template parameter T that you can define later to be float or double.

template<class T>
class Vector2d {
public:
    /*The x and y values are public to give easier access for
      outside funtions. Accessors and mutators are not really
      necessary*/
    T x;
    T y;

Second, always initialize class members in the constructor member-initaliation-list:

    //Constructor assigns the inputs to x and y.
    Vector2d(): x(T(0)), y(T(0)) {}
    Vector2d(const& T vx, const& T vy): x(vx), x(vy) {}

Third, I would change your arithmetic operators to compound assignment operators that are non-const and return by reference

    /*The following operators simply return Vector2ds that
      have operations performed on the relative (x, y) values*/
    Vector2d& operator+=(const Vector2d& v) { x += v.x; y += v.y; return *this; }
    Vector2d& operator-=(const Vector2d& v) { x -= v.x; y -= v.y; return *this; }
    Vector2d& operator*=(const Vector2d& v) { x *= v.x; y *= v.y; return *this; }
    Vector2d& operator/=(const Vector2d& v) { x /= v.x; y /= v.y; return *this; }

Fourth, I would the equality operator as a friend function that is symmetric in its arguments, and add the missing operator!=. Furthermore, I'd define operator== in terms of the C++11 std::tuple using the std::tie helper and the other operator in terms of this one

    //Check if the Vectors have the same values (uses pairwise comparison of `std::tuple` on the x,y values of L and R.
    friend bool operator==(const Vector2d& L, const Vector2d& R) { return std::tie(L.x, L.y) == std::tie(R.x, R.y); }
    friend bool operator!=(const Vector2d& L, const Vector2d& R) { return !(L == R); }

Fifth, if you insist on having comparison operators <, then I would not compare norms. Why? One reason is that you already define a norm function (Magnitude) later on, and you could simply compare vector norms directly. Furthermore, it is not a natural mathematical ordering. I would personally prefer (but it's a matter of taste!) use the natural ordering defined by std::tuple that does lexicograpical comparison on the x and then the y values:

    //Check if the Vectors have the same values (uses pairwise comparison of `std::tuple` on the x,y values of L and R.
    friend bool operator< (const Vector2d& L, const Vector2d& R) { return std::tie(L.x, L.y) < std::tie(R.x, R.y); }
    friend bool operator>=(const Vector2d& L, const Vector2d& R) { return !(L < R); }
    friend bool operator> (const Vector2d& L, const Vector2d& R) { return   R < L ; }
    friend bool operator<=(const Vector2d& L, const Vector2d& R) { return !(R < L); }

Sixth, the unary minus operator is fine as it is

    //Negate both the x and y values.
    Vector2d operator-() const { return Vector2d(-x, -y); }

Seventh, the scalar compound assignment should be non-const and return-by-reference

    //Apply scalar operations.
    Vector2d& operator*=(const& T s) { x *= s; y *= s; return *this; }
    Vector2d& operator/=(const& T s) { x /= s; y /= s; return *this; }
};

Note that I define all class member functions inside the class definition. This is mainly done for brevity and to avoid repetitive coding. You can also split up the declaration and definition, but since I suggest to make a class template, those member function definitions also need to be in a header file. If you decide to keep it a regular class, then you can put the member definitions in a source file (you could still follow the other suggestions).

Rest of the class interface

First, you could define your original arithmetic members as non-member function templates that are symmetric in their arguments and are one-liners forwarding to the corresponding member compound assignments

template<class T> Vector2d<T> operator+(const Vector2d<T>& L, const Vector2d<T>& R) { return Vector2d<T>(L) += R; }
template<class T> Vector2d<T> operator-(const Vector2d<T>& L, const Vector2d<T>& R) { return Vector2d<T>(L) -= R; }
template<class T> Vector2d<T> operator*(const Vector2d<T>& L, const Vector2d<T>& R) { return Vector2d<T>(L) *= R; }
template<class T> Vector2d<T> operator/(const Vector2d<T>& L, const Vector2d<T>& R) { return Vector2d<T>(L) /= R; }

Second, you also define your original 2 scalar member functions as 4 scalar non-member function templates so that you can both left- and right-multiply (your version only allowed right-multiplication by a scalar). Again these are one-liners forward to the corresponding member compound assignments

template<class T> Vector2d<T> operator*(const T& s, const Vector2d<T>& v) { return Vector2d<T>(v) *= s; }
template<class T> Vector2d<T> operator*(const Vector2d<T>& v, const T& s) { return Vector2d<T>(v) *= s; }
template<class T> Vector2d<T> operator/(const T& s, const Vector2d<T>& v) { return Vector2d<T>(v) /= s; }
template<class T> Vector2d<T> operator/(const Vector2d<T>& v, const T& s) { return Vector2d<T>(v) /= s; }

#endif

Note that these non-member function should also be defined right inside the header where you have your class definitions. Why? this way users of Vector2d<T> only need to include the header that contains the entire class interface.

Utility functions

First, the remain static member functions from your original class definition should be defined as non-member function templates because they can be implemented entirely in terms of the public member functions and the other non-member functions.

//Product functions
template<class T> T DotProduct(const Vector2d<T>&, const Vector2d<T>&);
template<class T> T CrossProduct(const Vector2d<T>&, const Vector2d<T>&);

Second, rename Magnitude to the acutal algorithm used to compute it (because there are many other norms used in geometry)

//Returns the length of the vector from the origin.
template<class T> T EuclideanNorm(const Vector2d<T>&);

The rest of the utility functions look fine at first glance:

//Return the unit vector of the input
template<class T> Vector2d<T> Normal(const Vector2d<T>&);

//Return a vector perpendicular to the left.
template<class T> Vector2d<T> Perpendicular(const Vector2d<T>&);

//Return true if two line segments intersect.
template<class T> bool Intersect(const Vector2d<T>&, const Vector2d<T>&, const Vector2d<T>&, const Vector2d<T>&);

//Return the point where two lines intersect.
template<class T> Vector2d<T> GetIntersect(const Vector2d<T>&, const Vector2d<T>&, const Vector2d<T>&, const Vector2d<T>&);

I have not much to say on the implementation of these utility functions, except that you can see that they are all using the extended class interface (both member and non-member functions). Again you can decide for yourself if you want to make them function templates or not, but in any case I would put them in a seperate header (or header + source file for non-templates) so that only users that are actually interested in such geometry functionality can include it.

Notes for further reading

The Boost.Operators library can automate most of the work for you (notice that there is a lot of repetition and similarity for the various arithmetic and relational operators? As always, familiarize yourself with Boost!