13
\$\begingroup\$

I have been learning C++ now for 2 months and this week I started reading a book on 3D graphics. I like coding whatever mathematical stuff I learn so I can understand it better, so when I learnt about Vectors, I decided to write a class on it.

I'd be grateful for any suggestions on my code, be it style-wise, performance-wise, or anything at all.

Some considerations I took when writing this code were:

I use inline functions because I heard that the C++ compiler, while smart, will not be able to inline everything that I want to be inlined automatically, even if I give hints.

I use commenting style that takes a lot of extra space. For me personally, it aids me in reading and documenting my code step by step.

I heard use of 'friend' operator is discouraged, but I seem to like using it. It allows me to code functions that, while could work as methods (e.g. vector.CrossProduct(otherVector)) sound better as functions CrossProduct(vector1, vector2) in my opinion.

I don't comment the implementation code. It seems too trivial to comment, I wonder if you think this is the case too?

//**********************************************************************
//* Vector.h
//* 
//* Just Another Vector Implementation (JAVI)
//**********************************************************************

#ifndef __VECTOR_H__
#define __VECTOR_H__

#include "Math.h"
#include <ostream>

class Vector
{
public:
    //******************************************************************
    //* Constructors
    //******************************************************************
    // Default Constructor
    //------------------------------------------------------------------
    // Sets the x, y and z components of this Vector to zero.
    //------------------------------------------------------------------
    Vector ();
    //------------------------------------------------------------------
    // Component Constructor
    //------------------------------------------------------------------
    // Sets the x, y and z components of this Vector to corresponding 
    // x, y and z parameters.
    //------------------------------------------------------------------
    Vector (float x, float y, float z);
    //------------------------------------------------------------------
    // Copy Constructor
    //------------------------------------------------------------------
    // Sets the x, y and z components of this Vector to equal the x, y 
    // and z components of Vector v.
    //------------------------------------------------------------------
    Vector (const Vector &v);
    //******************************************************************

    //******************************************************************
    //* Friend Operators
    //******************************************************************
    // Stream Insertion Operator
    //------------------------------------------------------------------
    // Writes the Vector v into the output stream in the format (x,y,z) 
    // so it can be used by various iostream functions.
    //------------------------------------------------------------------
    friend std::ostream &operator << (std::ostream &os, const Vector &v);
    //------------------------------------------------------------------
    // Equal To Operator
    //------------------------------------------------------------------
    // Compares the x, y and z components of Vector v1 and to the x, y 
    // and z components of Vector v2 and returns true if they are 
    // identical. Otherwise, it returns false.
    //------------------------------------------------------------------
    friend bool operator == (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // Not Equal To Operator
    //------------------------------------------------------------------
    // Compares the x, y and z components of Vector v1 and to the x, y 
    // and z components of Vector v2 and returns true if they are not 
    // identical. Otherwise, it returns false.
    //------------------------------------------------------------------
    friend bool operator != (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // Addition Operator
    //------------------------------------------------------------------
    // Adds the x, y and z components of Vector v1 to the x, y and z 
    // compenents of Vector v2 and returns the result.
    //------------------------------------------------------------------
    friend Vector operator + (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // Subtraction Operator
    //------------------------------------------------------------------
    // Subtracts the x, y and z components of Vector v2 to the x, y and 
    // z compenents of Vector v1 and returns the result.
    //------------------------------------------------------------------
    friend Vector operator - (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // Multiplication Operator
    //------------------------------------------------------------------
    // Multiplies the x, y and z components of Vector v with a scalar 
    // value and returns the result.
    //------------------------------------------------------------------
    friend Vector operator * (const Vector &v, float scalar);
    friend Vector operator * (float scalar, const Vector &v);
    //------------------------------------------------------------------
    // Division Operator
    //------------------------------------------------------------------
    // Divides the x, y and z components of Vector v with a scalar 
    // value and returns the result.
    //------------------------------------------------------------------
    friend Vector operator / (const Vector &v, float scalar);
    friend Vector operator / (float scalar, const Vector &v);
    //******************************************************************

    //******************************************************************
    //* Friend Functions
    //******************************************************************
    // DotProduct
    //------------------------------------------------------------------
    // Computes the dot product between Vector v1 and Vector v2 and 
    // returns the result.
    //------------------------------------------------------------------
    friend float DotProduct (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // CrossProduct
    //------------------------------------------------------------------
    // Computes the cross product between Vector v1 and Vector v2 and 
    // returns the result.
    //------------------------------------------------------------------
    friend Vector CrossProduct (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // Lerp
    //------------------------------------------------------------------
    // Returns a linear interpolation between Vector v1 and Vector v2 
    // for paramater t, in the closed interval [0, 1].
    //------------------------------------------------------------------
    friend Vector Lerp (const Vector &v1, const Vector &v2, float t);
    //------------------------------------------------------------------
    // Clamp - TODO: make this a method instead?
    //------------------------------------------------------------------
    // Clamps this Vector's x, y and z components to lie within min and 
    // max.
    //------------------------------------------------------------------
    friend Vector Clamp (const Vector &v1, float min, float max);
    //------------------------------------------------------------------
    // Min
    //------------------------------------------------------------------
    // Returns a Vector whos x, y and z components are the minimum 
    // components found in Vector v1 and Vector v2.
    //------------------------------------------------------------------
    friend Vector Min (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // Max
    //------------------------------------------------------------------
    // Returns a Vector whos x, y and z components are the maximum 
    // components found in Vector v1 and Vector v2.
    //------------------------------------------------------------------
    friend Vector Max (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // DistanceBetween
    //------------------------------------------------------------------
    // Returns the scalar distance between the Vector v1 and the Vector 
    // v2.
    //------------------------------------------------------------------
    friend float DistanceBetween (const Vector &v1, const Vector &v2);
    //------------------------------------------------------------------
    // DistanceBetweenSquared
    //------------------------------------------------------------------
    // Returns the scalar squared distance between the Vector v1 and 
    // the Vector v2.
    //------------------------------------------------------------------
    friend float DistanceBetweenSquared (const Vector &v1, const Vector &v2);
    //******************************************************************

    //******************************************************************
    //* Operators
    //******************************************************************
    // Copy Assignment Operator
    //------------------------------------------------------------------
    // Assigns this Vector's components to be equal to Vector v's 
    // components.
    //------------------------------------------------------------------
    Vector &operator = (const Vector &v);
    //------------------------------------------------------------------
    // Addition Assignment Operator
    //------------------------------------------------------------------
    // Adds to this Vector's components the components of Vector v.
    //------------------------------------------------------------------
    Vector &operator += (const Vector &v);
    //------------------------------------------------------------------
    // Subtraction Assignment Operator
    //------------------------------------------------------------------
    // Subtract from this Vector's components the components of Vector 
    // v.
    //------------------------------------------------------------------
    Vector &operator -= (const Vector &v);
    //------------------------------------------------------------------
    // Multiplication Assignment Operator
    //------------------------------------------------------------------
    // Multiply this Vector's components by a scalar value.
    //------------------------------------------------------------------
    Vector &operator *= (float scalar);
    //------------------------------------------------------------------
    // Division Assignment Operator
    //------------------------------------------------------------------
    // Divide this Vector's components by a scalar value.
    //------------------------------------------------------------------
    Vector &operator /= (float scalar);
    //------------------------------------------------------------------
    // Unary Minus Operator
    //------------------------------------------------------------------
    // Negate the components of this Vector.
    //------------------------------------------------------------------
    Vector &operator - ();
    //------------------------------------------------------------------
    // Array Subscript Operator
    //------------------------------------------------------------------
    // Allows access to the x, y and z components through an array 
    // subscript notation.
    //------------------------------------------------------------------
    float &operator [] (int i);
    //******************************************************************

    //******************************************************************
    //* Methods
    //******************************************************************
    // X
    //------------------------------------------------------------------
    // Returns the x component of this Vector.
    //------------------------------------------------------------------
    float X ();
    //------------------------------------------------------------------
    // Y
    //------------------------------------------------------------------
    // Returns the y component of this Vector.
    //------------------------------------------------------------------
    float Y ();
    //------------------------------------------------------------------
    // Z
    //------------------------------------------------------------------
    // Returns the z component of this Vector.
    //------------------------------------------------------------------
    float Z ();
    //------------------------------------------------------------------
    // Set
    //------------------------------------------------------------------
    // Sets the x, y and z components of this Vector to the paramaters 
    // of this function.
    //------------------------------------------------------------------
    void Set (float x, float y, float z);
    //------------------------------------------------------------------
    // MakeZero
    //------------------------------------------------------------------
    // Sets the x, y and z components of this Vector to zero.
    //------------------------------------------------------------------
    void MakeZero ();
    //------------------------------------------------------------------
    // IsZero
    //------------------------------------------------------------------
    // Returns true if the x, y and z components of this Vector are 
    // equal to zero.
    //------------------------------------------------------------------
    bool IsZero ();
    //------------------------------------------------------------------
    // LengthSquared
    //------------------------------------------------------------------
    // Returns the magnitude of the x, y and z components squared.
    //------------------------------------------------------------------
    float LengthSquared ();
    //------------------------------------------------------------------
    // Length
    //------------------------------------------------------------------
    // Returns the magnitude of the x, y and z components.
    //------------------------------------------------------------------
    float Length ();
    //------------------------------------------------------------------
    // Normalize
    //------------------------------------------------------------------
    // Sets the components of this Vector in such a way that their 
    // magnitude is equal to one.
    //------------------------------------------------------------------
    void Normalize ();
    //------------------------------------------------------------------
    // IsNormalized
    //------------------------------------------------------------------
    // Compares the magnitude of this Vector to one.
    //------------------------------------------------------------------
    bool IsNormalized ();
    //******************************************************************

private:
    //******************************************************************
    //* Private Member Variables
    //******************************************************************
    // x
    //------------------------------------------------------------------
    // The x component of this Vector.
    //------------------------------------------------------------------
    float x;
    //------------------------------------------------------------------
    // y
    //------------------------------------------------------------------
    // The y component of this Vector.
    //------------------------------------------------------------------
    float y;
    //------------------------------------------------------------------
    // z
    //------------------------------------------------------------------
    // The z component of this Vector.
    //------------------------------------------------------------------
    float z;
    //******************************************************************
};

inline Vector::Vector() : x(0.0f), y(0.0f), z(0.0f) {}

inline Vector::Vector(float x, float y, float z) : x(x), y(y), z(z) {}

inline Vector::Vector(const Vector &v) : x(v.x), y(v.y), z(v.z) {}

inline std::ostream &operator<<(std::ostream &os, const Vector &v)
{
    os << '(' << v.x << ',' << v.y << ',' << v.z << ')';
    return os;
}

inline bool operator==(const Vector &v1, const Vector &v2)
{
    return (AreEqual(v1.x, v2.x) &&
            AreEqual(v1.y, v2.y) &&
            AreEqual(v1.z, v2.z));
}

inline bool operator!=(const Vector &v1, const Vector &v2)
{
    return (!AreEqual(v1.x, v2.x) ||
            !AreEqual(v1.y, v2.y) ||
            !AreEqual(v1.z, v2.z));
}

inline Vector operator+(const Vector &v1, const Vector &v2)
{
    return Vector(v1.x+v2.x, v1.y+v2.y, v1.z+v2.z);
}

inline Vector operator-(const Vector &v1, const Vector &v2)
{
    return Vector(v1.x-v2.x, v1.y-v2.y, v1.z-v2.z);
}

inline Vector operator*(const Vector &v, float scalar)
{
    return Vector(v.x*scalar, v.y*scalar, v.z*scalar);
}

inline Vector operator*(float scalar, const Vector &v)
{
    return Vector(v.x*scalar, v.y*scalar, v.z*scalar);
}

inline Vector operator/(const Vector &v, float scalar)
{
    assert(!EqualsZero(scalar));
    scalar = 1.0f / scalar;
    return Vector(v.x*scalar, v.y*scalar, v.z*scalar);
}

inline Vector operator/(float scalar, const Vector &v)
{
    assert(!EqualsZero(scalar));
    scalar = 1.0f / scalar;
    return Vector(v.x*scalar, v.y*scalar, v.z*scalar);
}

inline float DotProduct (const Vector &v1, const Vector &v2)
{
    return v1.x * v2.x + v1.y * v2.y + v1.z * v2.z;
}

inline Vector CrossProduct(const Vector &v1, const Vector &v2)
{
    return Vector(v1.y*v2.z - v1.z*v2.y,
                  v1.z*v2.x - v1.x*v2.z,
                  v1.x*v2.y - v1.y*v2.x);
}

inline Vector Lerp(const Vector &v1, const Vector &v2, float t)
{
    return Vector(Lerp(v1.x, v2.x, t),
                  Lerp(v1.y, v2.y, t),
                  Lerp(v1.z, v2.z, t));
}

inline Vector Clamp(const Vector &v, float min, float max)
{
    return Vector(Clamp(v.x, min, max),
                  Clamp(v.y, min, max),
                  Clamp(v.z, min, max));
}

inline Vector Min(const Vector &v1, const Vector &v2)
{
    return Vector(Min(v1.x, v2.x),
                  Min(v1.y, v2.y),
                  Min(v1.z, v2.z));
}

inline Vector Max(const Vector &v1, const Vector &v2)
{
    return Vector(Max(v1.x, v2.x),
                  Max(v1.y, v2.y),
                  Max(v1.z, v2.z));
}

inline float DistanceBetween(const Vector &v1, const Vector &v2)
{
    Vector distance = v1 - v2;
    return distance.Length();
}

inline float DistanceBetweenSquared (const Vector &v1, const Vector &v2)
{
    Vector distance = v1 - v2;
    return distance.LengthSquared();
}

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

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

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

inline Vector &Vector::operator*=(float scalar)
{
    x *= scalar;
    y *= scalar;
    z *= scalar;
    return *this;
}

inline Vector &Vector::operator/=(float scalar)
{
    assert(!EqualsZero(scalar));
    scalar = 1.0f / scalar;
    x *= scalar;
    y *= scalar;
    z *= scalar;
    return *this;
}

inline Vector &Vector::operator-()
{
    x = -x;
    y = -y;
    z = -z;
    return *this;
}

inline float &Vector::operator[](int i)
{
    if (i == 0) {
        return x;
    } else if (i == 1) {
        return y;
    } else if (i == 2) {
        return z;
    } else {
        assert("[] Access error!");
    }
}

inline float Vector::X()
{
    return x;
}

inline float Vector::Y()
{
    return y;
}

inline float Vector::Z()
{
    return z;
}

inline void Vector::Set(float x, float y, float z)
{
    this->x = x;
    this->y = y;
    this->z = z;
}

inline void Vector::MakeZero()
{
    x = y = z = 0.0f;
}

inline bool Vector::IsZero()
{
    return EqualsZero(x) &&
           EqualsZero(y) &&
           EqualsZero(z);
}

inline float Vector::LengthSquared()
{
    return x*x + y*y + z*z;
}

inline float Vector::Length()
{
    return Sqrt(LengthSquared());
}

inline void Vector::Normalize()
{
    float magnitude = Length();
    assert(!EqualsZero(magnitude));

    magnitude = 1.0f / magnitude;

    x *= magnitude;
    y *= magnitude;
    z *= magnitude;
}

inline bool Vector::IsNormalized()
{
    return AreEqual(Length(), 1.0f);
}

#endif
\$\endgroup\$

7 Answers 7

16
\$\begingroup\$
  • Comments

    Honestly, too many of them.

  • Object-orientation

    I don't see why vector components are made private. A client can freely and independently modify them via Set method. There's no internal state to maintain, no invariant to protect. I recommend to make them public and eliminate Set(), X(), Y(), Z() methods altogether.

  • Math

    I am quite surprised by the presence of operator/(float, Vector). There is no immediately obvious value in it. Mathematically such operation makes no sense, and dividing a scalar by a vector shall be flagged as error ASAP.

    On the other hand, dot product would very naturally be

    float operator*(Vector&, Vector&)
    

    There is no implementation of AreEquals and EqualsZero methods. In any case, I'd expect IsZero method to compare a norm rather than individual components. It would actually be nice to abstract a norm calculation (right now the client is forced to Euclidean distance).

  • Implementation details

    It is strongly recommended to express operator != in terms of operator ==. Otherwise, you face with a double maintenance problem, and the reader should do extra work to make sure that the semantics of comparisons is correct.

    Similarly, other operators with tightly bound semantics also should not be independent. It is usually recommended to express operator+ in terms of operator+=, etc. For more details, see a Canonical Implementation section in C++ reference.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ To indent text within a bullet point, add two spaces before the start of the paragraph. \$\endgroup\$
    – Jamal
    Sep 12, 2014 at 17:55
  • 1
    \$\begingroup\$ Hm. Why dot-product instead of cross-product? Actually, as there are two good possibilities for one operator, neither should be chosen, but both offered as a normal function. \$\endgroup\$ May 6, 2019 at 14:02
13
\$\begingroup\$

Firstly, even though your commenting style may help you read your code, it is definitely overly verbose in places. Comments like:

 // Default Constructor
 ...
 Vector()

are obvious, and more experienced C++ programmers would likely find it bothersome.

In regards to inline functions:

I use inline functions because I heard that the C++ compiler, while smart, will not be able to inline everything that I want to be inlined automatically, even if I give hints.

This isn't really true. Regardless, even using inline is just a suggestion to the compiler - one it is free to ignore at its leisure (it can make a difference to the linking stage, however). Microsoft's compiler (MSVC) even has an extension __forceinline, which is still just a suggestion that it is free to ignore. Compilers do relatively sophisticated cost/benefit tradeoff analysis these days with regards to inlining functions, regardless.

A minor point before we look at the implementation: anything beginning (or ending) with __ is reserved for compiler usage, so don't use things like:

 #ifndef __VECTOR_H__

Basically, the (general) rule in C++ is to not prefix anything with _ or __. Use a single trailing _ if you'd like.


For the implementation itself:

You don't need to write a copy constructor:

Vector (const Vector &v);

The compiler will happily generate this for you (and it will be entirely correct in this case as well). You might want to have a read about the rule of three. Effectively, if you haven't allocated any memory with new, you don't need to write a copy constructor / destructor / copy assignment operator.

Is there any particular reason you're using floats? Most modern day x86(-64) hardware is actually optimized for doubles, and using them will often actually be faster.

Your capitalization is a bit odd. Generally, either you'd use PascalCase for classes and either camelCase or snake_case for methods/functions. Having capitalized function names is a bit odd, especially for single character names like:

float X ();
float Y ();
float Z ();

Speaking of these accessors, they should all be const, as they don't change anything about the vector:

float x() const;
float y() const;
float z() const;

You are definitely overusing friend functions here, and you don't need to. Anywhere you are accessing the fields directly (such as v1.x or v2.y) can be replaced with the (public) accessor function (v1.x() and v1.y()). This would allow you to remove having to use friend just about everywhere in your code.

Code that overloads the basic operators (+, -, *, /) and their in-place counterparts (+=, etc) can generally leverage each other. For example:

// This actually needs to be a friend if you define it as a free
// function as it needs to modify (and not just read) x, y, z
friend Vector& operator+=(Vector& v1, const Vector& v2)
{
    v1.x += v2.x;
    v1.y += v2.y;
    v1.z += v2.z;
    return v1;
}

You can then define operator+ in terms of this:

Vector operator+(const Vector& v1, const Vector& v2)
{
     Vector v(v1);
     v += v2;
     return v;
}

Having an overloaded operator[] for a 3-element vector seems like an odd design choice, and something that could easily be confused.

\$\endgroup\$
5
  • \$\begingroup\$ Just a note: From what I know, _UPPER and __anything and whatewer__two__unerscores__anywhere is not allowed, but _first_small is allowed. Am I wrong? \$\endgroup\$
    – user52292
    Sep 12, 2014 at 19:45
  • \$\begingroup\$ You could also let the compiler produce the copy in operator+ by passing the first parameter by-value. Doesn't make a difference here, though. \$\endgroup\$
    – dyp
    Sep 12, 2014 at 19:51
  • \$\begingroup\$ @firda There are rules about a single leading underscore. Basically, it is allowed in some situations, but the rules are complicated, and I find it's better to just generally avoid it. \$\endgroup\$
    – Yuushi
    Sep 13, 2014 at 5:30
  • \$\begingroup\$ "hardware is actually optimized for doubles", a little misleading. Modern hardware is optimized for either case; 4 floats (3 for the vec3 and a blank) or 2 doubles (and another instruction for the other). \$\endgroup\$ Sep 13, 2014 at 6:26
  • \$\begingroup\$ @CoffeeandCode Well, perhaps slightly misleading. But the fact is also that the first 6 arguments in x86-64 will be use registers directly, so passing 2 x 3D vectors will use registers and no stack space. Either way, there's almost 0 reason to use floats unless you've profiled and are completely certain there's some kind of bottleneck. \$\endgroup\$
    – Yuushi
    Sep 13, 2014 at 6:41
5
\$\begingroup\$

Answers to Questions

I use inline functions because I heard that the C++ compiler, while smart, will not be able to inline everything that I want to be inlined automatically, even if I give hints.

The inline keyword is not used by any modern compiler to decide weather to do inlining. The compiler is much smarter than you on deciding when to use and when not to do it anyway. It is required where you use (in a header file when not part of the class).

I use commenting style that takes a lot of extra space. For me personally, it aids me in reading and documenting my code step by step.

It's a bit much. I don't mind the comments that tell you what the function does. But things that are obvious you should leave out (like: Default Constructor, Component Constructor and Copy Constructor seem superfluous).

Also commenting stuff that should be obvious.

// Equal To Operator
//------------------------------------------------------------------
// Compares the x, y and z components of Vector v1 and to the x, y 
// and z components of Vector v2 and returns true if they are 
// identical. Otherwise, it returns false.
//------------------------------------------------------------------
friend bool operator == (const Vector &v1, const Vector &v2);

I would only have written that comment of operator== did something that was not obvious. If it does what you expect then there seems little point in writing it down.

I heard use of 'friend' operator is discouraged, but I seem to like using it. It allows me to code functions that, while could work as methods (e.g. vector.CrossProduct(otherVector)) sound better as functions CrossProduct(vector1, vector2) in my opinion.

I like the friend operator. I think it adds clarity and decreases coupling.
https://softwareengineering.stackexchange.com/a/99595/12917 . You have more than I would normally see. But in this case I have no issues with them.

But you have to be careful. The way you have written things will tend to hide errors in certain situations. Currently you are OK because you don't have a single argument constructor. But what happens if somebody added one (in a few years they wanted a convenient way of creating a simple Vector along the X-access.

 // Somebody adds.
 Vector::Vector(float x) : x(x), y(0), z(0) {}

 // Now this code
 Vector   x(4,5,6);

 if (x == 3) // Here you get an automatic conversion. (Not that unexpected)
 {}

 if (3 == x) // Here is also an automatic conversion. (Can be unexpected).
             // But if the operator had been a member rather than a friend
             // then this would not have been an automatic conversion and
             // generated a compiler error.

I am not saying it is bad or good but you need to watch for that kind of thing and see if that is what you expect your objects to work.

I don't comment the implementation code. It seems too trivial to comment, I wonder if you think this is the case too?

I agree. If I can read the code and see what is happening no comments are needed. If you are implementing a specific algorithm provide a link to the algorithm etc..

Code review.

The others have covered most of the points I would have:

#ifndef __VECTOR_H__

I want to emphasis this one (it happens a lot with beginners). Double underscore is reserved anywhere in an identifier never use it. Underscore at the beginning of an identifier is reserved in certain contexts (the rules are complex). So best to never use an identifier that begins with an underscore (most people get it wrong if they try).

The name Vector is already being used (though the capitalization is different). To make sure people spot the difference I would make sure you put this in your own namespace;

 namespace ThreeDimension
 {
     class Vector{};
 }
 int main()
 {
      namespace TD = ThreeDimension;
      TD::Vector  data;
 }
\$\endgroup\$
8
  • 2
    \$\begingroup\$ "You have more than I would normally see. But in this case I have no issues with them." None of those friend functions need to be friends, as far as I can see. \$\endgroup\$
    – dyp
    Sep 12, 2014 at 19:45
  • \$\begingroup\$ @dyp: But I still have no issue with them being friends (as its documenting a tight coupling). \$\endgroup\$ Sep 12, 2014 at 21:36
  • 1
    \$\begingroup\$ I do not see why they should be tightly coupled. If Vector provides a public interface to access its components, anyone can implement those functions. They do not need to be some part of the class nor do they require any privileged access. \$\endgroup\$
    – dyp
    Sep 12, 2014 at 23:14
  • \$\begingroup\$ @dyp. But it does not (provides a public interface to access its components) you would have to use getters (which I really hate). The members are private. I don't personally see any issue here. But neither do I see any point arguing about. If some other reviewer on the team was putting up a fight about making them non friend then I would not bother to argue against them either (I would let the author justify it). \$\endgroup\$ Sep 12, 2014 at 23:30
  • 2
    \$\begingroup\$ I agree. But then, what do you mean with "One implementation depends on the other"? and "The members are private."? The guideline I'm trying to apply here is: Functions that do not need access to private members should not have privileged access. Implementing free functions that do not require privileged access as friends is possible, and sometimes useful (templates), but IMHO there's no benefit here and it just bloats the class (more text). \$\endgroup\$
    – dyp
    Sep 12, 2014 at 23:49
2
\$\begingroup\$

I agree with a previous answer that suggest making your x, y, z variables public. Also, in my past experience (3d game programming), I usually declare my components as a union within the class, so you have the freedom to access the components specifically or as in an array (without the overhead of a function call). The same concept can be applied when you start making matrix classes (something you will certainly be doing if rolling your own 3d math library), and you can set up a union to map your data as an array, individual components, and even the component vectors, all within the same memory.

union
{
    float v[3];
    struct
    {
        float x, y, z;
    };

};

Edit: In the comments, dyp points out that anonymous structs are not part of the c++ standard. I looked into this, and found this question which not only confirms dyp's assertion, but indicates that the usage I suggested above is undefined behavior in it's entirety, so you may not want to do this, though I have released several full 3D games over the years that use exactly this functionality. Apparently there is always more to learn...

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Anonymous structs are language extensions, they're not supported in standard C++. \$\endgroup\$
    – dyp
    Sep 12, 2014 at 19:49
  • 1
    \$\begingroup\$ @dyp: Woohaa, didn't know that, I use them so long that I didn't even considered them not being standardized. \$\endgroup\$
    – user52292
    Sep 12, 2014 at 19:52
2
\$\begingroup\$

Something to add to the other answers:

  • why are you redefining math operators on floats? (I mean Max, Min, AreEqual...) I cannot imagine any reason why the standard library max, min, == are not good for you.

  • Math.h is a bad name for a custom header. If your code will be ported to some case insensitive filesystem (notably: MacOS or Windows) you would hide the standard math.h (even if in C++ you have cmath I would discourage this).

  • if you want your library to be user-friendly you should stick to names conventions largely used. Use same function names used for float and look at the standard complex implementation for suggestions. I would use sqr for LengthSquared and abs=sqrt(sqr).

  • more generally, you are implementing too many functions. Write the fundamental operations and let the user compose them to obtain what he needs:

    DistanceBetween(v,w) -> abs(v-w)
    DistanceBetweenSquared(v,w) -> sqr(v-w)
    normalize(v) -> v /= abs(v)
    v.MakeZero() -> v = Vector()
    v.Set(x,y,z) -> v = Vector(x,y,z)
    v.isNormalized() -> abs(v) == 1.0
    
\$\endgroup\$
2
  • \$\begingroup\$ AreEqual is used to compare two floats with some leniency value, to account for arithmetic error. It's based off something similar from here: floating-point-gui.de/errors/comparison Also, I have many wrapper functions in Math.h that will do some assertions on the data, before returning the equivalent math.h function. \$\endgroup\$
    – user52988
    Sep 13, 2014 at 17:35
  • 2
    \$\begingroup\$ I have suspected that... but I would say that this is even worse because you are changing the expected behaviour of the equality operator. For example: it is not true that a==b and b==c implies a==c. If one wants to check approximate equality should make it very explicit... \$\endgroup\$ Sep 13, 2014 at 17:51
1
\$\begingroup\$

All you need to make a sensible vector class is this:

class vector {
    float e[3];
public:
    vector(float a = 0, float b = 0, float c = 0)  : e[0](a), e[1](b), e[2](c) { }
    float& operator(int i) { return e[i]; }
    float  operator(int i) const { return e[i]; }
    static int const size;
    /* repeat for -, * and / -- same pattern */
    vector& operator += (vector const& b) {
       e[0] += b[0]; e[1] += b[1]; e[2] += b[2];
       return *this;
    }
};

All your binary operators can (and should) be declared outside of the class, taking const arguments and returning a const value, like so:

vector const operator + (vector const& a, vector const& b) {
    return vector(a) += b;
}

For the dot product, length, angles and such, define functions which take const arguments and simply use the [] operator. You could use a template implementation so you could reuse those functions for any size vector.

template<typename T>
float sum(T a) {
    float s = 0;
    for (int i = 0; i < a.size; ++i) s += a[i];
    return s;
}

template<typename T>
float dot(T a, T b) {
    return sum(a * b);
}

template<typename T>
float length(T a) {
     return sqrt(dot(a, a));
}

Note that the * operator should multiply each element of the vectors and store them in the result vector, by element -- this is not the dot product.

You may also want to add a set of comparison operators, as you see fit.

The reason for having the [] operator is that it allows us to access the object as a const while also allowing us to use the same operator (when appropriate) as non-const.

I have included the static const variable size to allow iteration over vectors of any size. You will need a line in some file which defines the variable:

int const vector::size = 3;

Remember to only have that line in a source file (i.e. not a header file).

Anonymous structs is a standard feature from C++11 onwards -- if you have a modern compiler, you should be able to use anonymous structs as much as you'd like.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ float& operator(int i) is probably a typo: either operator()(int i) or operator[](int i), i.e., the operator you're trying to overload is missing. \$\endgroup\$
    – dyp
    Sep 12, 2014 at 23:51
1
\$\begingroup\$

I'd like to add that a very important feature of C++ is templates. You can parameterize your vector by element types.

Example:

template <typename T>
struct Vector3 { 
    T x, y, z; 
};

There are other suggestions here, they include:

  • Parameterize vector/matrix structs by element type and dimension.
  • Provide specializations and typedefs for the most common types and sizes, for convenience.
  • Provide a nice, full set of constructors, overloaded operators and free functions (like dot, cross, etc). Seriously, go crazy with these and implement everything you can think of.
  • Don’t try to build SIMD into your general-purpose vector/matrix structs. Do use SIMD judiciously, where it’s effective, and keep the relevant data in SOA layout.
  • Consider adding type-safe affine math (points and affine transformations) on top of the basic linear algebra features.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.