patterncppModerate
Review of 2d Vector class
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
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
First, I would change
Second, always initialize class members in the constructor member-initaliation-list:
Third, I would change your arithmetic operators to compound assignment operators that are non-const and return by reference
Fourth, I would the equality operator as a friend function that is symmetric in its arguments, and add the missing
Fifth, if you insist on having comparison operators `
The rest of the utility functions look fine at first glance:
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!
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.