9
\$\begingroup\$

Pretty simple 2-Dimensional Vector template with operators and two utility functions making use of C++20 concepts. Header-only templates, function inlining and operator overloading, etc. is not exactly my strong foot, so that's why i wrote this hoping to get some feedback.

#pragma once

#include <cmath>
#include <type_traits>

namespace math {

    template<typename T>
    concept Arithmetic = std::is_arithmetic_v<T>;

    template<Arithmetic T>
    struct Vector2D
    {
        T X = 0;
        T Y = 0;

        Vector2D() = default;

        Vector2D(T x, T y);

        Vector2D(const Vector2D<T> &other);

        inline T Magnitude() const;

        inline Vector2D<T> Normal() const;
    };

    template<Arithmetic T>
    inline Vector2D<T>::Vector2D(T x, T y) : X(x), Y(y)
    {}

    template<Arithmetic T>
    inline Vector2D<T>::Vector2D(const Vector2D<T> &other) : X(other.X), Y(other.Y)
    {}

    template<Arithmetic T>
    inline T Vector2D<T>::Magnitude() const
    {
        return std::sqrt(X * X + Y * Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> Vector2D<T>::Normal() const
    {
        auto magnitude = Magnitude();

        return Vector2D<T>(X / magnitude, Y / magnitude);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator-(const Vector2D<T> &vec)
    {
        return Vector2D<T>(-vec.X, -vec.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator+(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator-(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator*(const Vector2D<T> &vec, const T factor)
    {
        return Vector2D<T>(vec.X * factor, vec.Y * factor);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator*(const T factor, const Vector2D<T> &vec)
    {
        return Vector2D<T>(factor * vec.X, factor * vec.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> operator/(const Vector2D<T> &vec, const T divisor)
    {
        return Vector2D<T>(vec.X / divisor, vec.Y / divisor);
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator+=(Vector2D<T> &first, const Vector2D<T> &second)
    {
        first.X += second.X;
        first.Y += second.Y;

        return first;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator-=(Vector2D<T> &first, const Vector2D<T> &second)
    {
        first.X -= second.X;
        first.Y -= second.Y;

        return first;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator*=(Vector2D<T> &vec, const T factor)
    {
        vec.X *= factor;
        vec.Y *= factor;

        return vec;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator/=(Vector2D<T> &vec, const T factor)
    {
        vec.X /= factor;
        vec.Y /= factor;

        return vec;
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator==(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return (first.X == second.X) && (second.Y == second.Y);
    }

    template<Arithmetic T>
    inline Vector2D<T> &operator!=(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return !(first == second);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Seems an almost trivial extension to make this a templated N-dimensional vector. \$\endgroup\$
    – throx
    Commented Feb 19, 2021 at 2:59
  • 1
    \$\begingroup\$ Your Normal function has a bug. If Magnitude returns zero you will crash with a divide by zero error. I would also argue that division is slow and instead you should pre-calculate the length to instead use inverse multiplication: const auto length = Magnitude(); if(length > T{0}) { const auto inv_length = T{1} / length; return {X * inv_length, Y * inv_length} } return {}; \$\endgroup\$
    – Casey
    Commented Feb 19, 2021 at 7:04

3 Answers 3

8
\$\begingroup\$

Overview

Your code as written is perfectly fine (apart from the one bug in subtraction). Anything mentioned here is mainly to help future readers.

You overcomplicated the design by adding constructors to Vector2D.
The default constructors work perfectly and as expected in this type of situation.

There is an argument to make operators members of the class rather than free standing functions. But either work.

Your use of reference marker & is very C-like. You put it next to the variable rather than next to the type. This is a very subjective style thing (usually dictated by your style guide). But C++ guides skew one way while C guides skew the other.

 void doStuff(C const& param)
              ^^^^^^^^   param has a type: C const&

 void doStuff(C& param)
              ^^         param has a type: C&

This has happened because in C++ types are much more significant and important. So we pay much more attention to type things in C++.

Code Review

I am going to have to take your word that this is how concepts work. I look forward to this becoming available more consistently.

    template<typename T>
    concept Arithmetic = std::is_arithmetic_v<T>;

    template<Arithmetic T>
    struct Vector2D
    {

Sure but not needed if you don't define other constructors (see below):

        Vector2D() = default;

Sure but not needed as the brace initializer works and does this for you:

        Vector2D(T x, T y);

Sure but not needed; the default copy constructor should work here:

        Vector2D(const Vector2D<T> &other);

You don't need to declare these inline here. It has no effect. You put the inline next to the method declaration to make sure that the compiler does not give you multiple definition warnings.

        inline T Magnitude() const;
        inline Vector2D<T> Normal() const;
    };

I would note that the inline keyword has nothing to do with in-lining the code. The compiler decides that without any help from the engineer (because it is better at it than you are).

I would have simplified that to:

    template<Arithmetic T>
    struct Vector2D
    {
        T  x;
        T  y;

        T Magnitude() const;
        Vector2D<T> Normal() const;
    };

Arithmetic Operators:

There is a debate about whether these should be members or free-standing functions. Either is fine. I will not ding you for doing it one way when I would have done it another.

There is an argument for making them free-standing functions. But this is usually based around symmetry when using the operators.

   R = X + Y;

If you use methods:

If X is Vector2D and Y is not then the compiler can potentially convert Y into Vector2D and still apply the operation. But if it is the other way around - Y is Vector2D and X is not - then the compiler cannot convert X to a Vector2D.

If you use free-standing functions:

Then if one parameter is Vector2D then the other can be converted to Vector2D and the function applied. This you have symmetrical conversions.

This is fine if the types are "Arithmetic" in nature and can easily convert from anything into your type; this is not the case here with Vector2D. So this argument does not stand. I would also argue that this applies to most types; you have to have a very good argument that your type should allow auto conversion (most types don't, and use explicit on one-argument constructors to prevent this).

Thus here you can use either technique and both are fine.

I personally would have made them members of the class (the sole reason is to add clarity so you can read the class and see all the things you can do to change them). You could argue you can have the same affect by adding all the declarations for arithmetic operations as friends to the class (or simply put the declarations directly under the class (and I could not argue against this).

Whatever way you decide, I would put the declarations beside the class and away from the definitions so it is easy to see all the possible operations that apply to the class.


Normally we implement operator+ in terms of operator+=.

    template<Arithmetic T>
    inline Vector2D<T> operator+(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }

I would write it like this:

    template<Arithmetic T>
    inline Vector2D<T>& operator+=(Vector2D<T>& first, Vector2D<T> const& second)
    {
        first.X += second.X;
        first.Y += second.Y;
        return first;
    }

    template<Arithmetic T>
    inline Vector2D<T> operator+(Vector2D<T> first, Vector2D<T> const& second)
    {
        // Note Pass first by value.
        // This automatically gets us a new version of the object.
        // The compiler will be able to detect if we can use move/copy
        // automatically to get this move version.
        //
        // We can then do the += on this new copy.
        // and perfectly return this value as output.
        return first += second;
    }

This seems like a bug:

    template<Arithmetic T>
    inline Vector2D<T> operator-(const Vector2D<T> &first, const Vector2D<T> &second)
    {
        // Should you not subtract here:
        return Vector2D<T>(first.X + second.X, first.Y + second.Y);
    }
\$\endgroup\$
12
  • 1
    \$\begingroup\$ Defining operator + that way is a bad habit. It doesn’t matter in this particular case, but friend X operator +(X left, X const& right) { return left += right; } copies or moves the left operand as appropriate, avoiding unnecessary copies in case it was an rvalue. \$\endgroup\$ Commented Feb 19, 2021 at 0:01
  • \$\begingroup\$ @RomanOdaisky I fixed the + operator. Did I get the description correct. \$\endgroup\$ Commented Feb 19, 2021 at 0:17
  • \$\begingroup\$ Yes, though the function body could have used a return. \$\endgroup\$ Commented Feb 19, 2021 at 0:36
  • \$\begingroup\$ By the way, C++20 does fix the symmetry problems with operators defined as member functions. \$\endgroup\$ Commented Feb 19, 2021 at 0:38
  • \$\begingroup\$ “By the way, C++20 does fix the symmetry problems with operators defined as member functions.” As far as I know, that’s only for equality/relational operators. And it applies both to member and non-member versions. \$\endgroup\$
    – indi
    Commented Feb 19, 2021 at 1:07
4
\$\begingroup\$

Martin York provided a great, in-depth answer. I agree with all of their points.

I want to mention a C++20 feature you may not be aware of, which is the new compiler-provided comparison operators. This blog post goes pretty in-depth on the details, but the highlights are:

  • Comparison operators in C++20 can be inferred by the compiler for a given type, based on operator== and operator<=> (the new "spaceship" operator).
    • Defining operator== for a type allows inequality (!=) to be inferred for that type.
    • Defining operator<=> for a type allows all comparison operators (<, <=, >, >=, ==, !=) to be inferred for that type.
  • The compiler-inferred definitions are both constexpr and noexcept!
  • Parameter ordering is also inferred (so if you define Foo::operator==(int), you get Foo == int, int == Foo, Foo != int, and int != Foo all in one).
  • Both of these operators can be defaulted. The default version will perform comparison on all class members in order of definition.

What does this mean for you? In your Vector2D type, you can declare bool operator==(const Vector2D<T>&) const = default; and the compiler will automatically generate both operator== and operator!=.
(Obviously, this only works if you can safely rely on the default comparison operators for your class members. In this case, your operator== and operator!= are already using default comparison on the class members, so this is not an issue for you.)

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I am aware about the operator<=>. From my understanding(haven't actually used it before) i would get all comparison operators including <, >, <=, >= from it. Since some of them don't make much sense for a Vector2 class i decided to not use it. Feel free to correct me if im wrong or if there is a way to supress unwanted operators. \$\endgroup\$
    – Eric
    Commented Feb 18, 2021 at 22:21
  • 2
    \$\begingroup\$ Yes, if you default operator<=>, you’d get all equality and comparison operators. But as @cariehl describes, you can just define operator== to get all variations of (in)equality and only (in)equality. If you don’t want relational ops, just don’t define operator<=>. \$\endgroup\$
    – indi
    Commented Feb 19, 2021 at 1:09
3
\$\begingroup\$
  1. Get rid of all constructors, the default ones do exactly the same, and given that C++20 includes P0960, you can initialize structs with parentheses.

  2. Fix the return type of operator == and default it.

  3. Get rid of operator !=, it’s generated automatically.

  4. std::hypot is a thing.

  5. If the return type isn’t auto, you can skip it in the return statement: return {-X, -Y};

  6. Express operators in terms of other operators where possible to minimize errors. Use @= to define @, use Vector * T to define T * Vector etc.

  7. In particular, use the X operator @(X left, X const& right) { return left @= right; } idiom, as it automatically uses the correct constructor, copy or move, for the left operand, as appropriate.

  8. Consider limiting the type T to floating point or you’ll get funny magnitudes and normed values.

  9. Finally, write some tests, they would have caught the operator - error, for example.

\$\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.