HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Review of 2d Vector class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
classreviewvector

Problem

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
#include

/*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;

//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 i

Solution

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 
#include 

/*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 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 ` 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 T DotProduct(const Vector2d&, const Vector2d&);
template T CrossProduct(const Vector2d&, const Vector2d&);


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 T EuclideanNorm(const Vector2d&);


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

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

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

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

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


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!

Code Snippets

#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.*/
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;
//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) {}
/*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; }
//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); }

Context

StackExchange Code Review Q#26608, answer score: 13

Revisions (0)

No revisions yet.