4
\$\begingroup\$

I'm brand new to cpp and tried implementing a simple Vector class that supports abs(), equality and addition operators. I just wanted a quick set of eyes to tell me if I'm doing anything egregious in my constructor/operators and finally their usage.

Note: the [[nodiscard]] was recommended by the clion IDE.

#include <iostream>
#include <cmath>

class Vector {
public:
    double x, y;
    [[nodiscard]] double abs() const {return sqrt(x*x + y*y);}

    // constructor
    Vector(double x, double y) {
        this->x = x;
        this->y = y;
    }

    // add op
    Vector operator+(const Vector& b) {
        Vector vec(this->x, this->y);
        vec.x += b.x;
        vec.y += b.y;
        return vec;
    }

    // eq op
    bool operator==(const Vector& b) {
        if (this->x == b.x && this->y == b.y) {
            return true;
        }
        return false;
    }
};

int main()
{
    Vector vec = Vector(4, 8);
    Vector vec2 = Vector(3.2, 5.5);
    Vector vec3 = vec + vec2;
    bool equal = vec == vec2;
    bool equal2 = vec == Vector(4, 8);

    std::cout << equal << std::endl;
    std::cout << equal2 << std::endl;
    std::cout << vec3.x << std::endl;
    std::cout << vec.abs() << std::endl;
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

For the add operator, you're currently constructing a brand new vector when you do Vector vec(this->x, this->y), before manipulating the newly constructed vector's x and y values. If this is what you want to do, you can add the values in the constructor instead: return Vector(x+other.x, y+other.y). You may, however, also want to implement and use the incremental add operator +=, where you instead just could add to this->x and this->y and return *this. If you do this you should also return by reference, i.e.:

Vector& operator+=(const Vector& other) {
  this->x += x;
  this->y += y;
  return *this;
}

You probably want your x and y members to be private and not public. And use member initialisation - your constructor can be changed to

Vector(double x, double y) : x(x), y(y) {}

Also consider that if (condition) return true else return false is the same as return condition, i.e. you can do

    bool operator==(const Vector& other) {
        return (this->x == other.x && this->y == other.y);
    }

Finally, maybe have a look at some existing libraries that implement vectors (in 2 or 3 dimensions - shouldn't matter much). These should be able to give you some help or inspiration.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thanks a lot for the advice, some of it is going over my head so I'll look some things up today and get back to you. \$\endgroup\$ Jan 5 at 12:26
  • 1
    \$\begingroup\$ Using member initialization (rather than assigning in the constructor's body) does not enable RAII. I don't see what RAII has to do with this class, at all. \$\endgroup\$
    – JDługosz
    Jan 11 at 15:47
  • 1
    \$\begingroup\$ You're right of course @JDługosz, don't know what I was thinking there. \$\endgroup\$
    – ades
    Jan 11 at 21:08
4
\$\begingroup\$

In abs(), we use global-namespace sqrt. You probably get away with this on a platform where <cmath> also provides that, but to be truly portable, don't rely on that - use its full name std::sqrt. Better still, we can use std::hypot(), which avoids potential problems when the sum of the squares exceeds the range of double.

The [[nodiscard]] annotation seems well-intentioned, but isn't really necessary - nobody is likely to use a const member function for side-effects and fail to use the result, and no harm comes from that, making the annotation arguably just clutter. The real benefit is for functions such as std::scanf() where the return value tends to be overlooked by users.


In the constructor, we should prefer to initialise the members, rather than assign them in the body:

Vector(double x, double y)
    : x{x},
      y{y}
{
}

However, we don't need a constructor at all, if we're happy to have a default constructor as well. We'll want our default constructor to create a useful default value - I'd recommend {0,0} - and we can achieve that by providing default values for x and y:

double x = 0;
double y = 0;

Consider providing a += operator. We can then use that to implement + as a non-member function:

    Vector& operator+=(const Vector& b) {
        x += b.x;
        y += b.y;
        return *this;
    }

// (outside the class)
Vector operator+(Vector a, const Vector& b) {
    return a += b;
}

Note that we accept a by value, so we can modify our copy of it, but b can be a const reference.


The equality operator should be const, and can be defaulted in current standard C++:

bool operator==(const Vector& b) const = default;

In the main() function, we're unnecessarily flushing each line with std::endl. Plain newline (\n) is more efficient.


Modified code

#include <cmath>

struct Vector
{
    double x = 0;
    double y = 0;

    bool operator==(const Vector& b) const = default;

    Vector& operator+=(const Vector& b) {
        x += b.x;
        y += b.y;
        return *this;
    }

    double abs() const
    {
        return std::hypot(x, y);
    }
};

Vector operator+(Vector a, const Vector& b) {
    return a += b;
}
#include <iomanip>
#include <iostream>

int main()
{
    auto vec = Vector{4, 8};
    auto vec2 = Vector{3.2, 5.5};
    auto vec3 = vec + vec2;
    bool equal = vec != vec2;
    bool equal2 = vec == Vector{4, 8};

    std::cout << std::boolalpha
              << equal << '\n'
              << equal2 << '\n'
              << vec3.x << '\n'
              << vec.abs() << '\n';
}
\$\endgroup\$
12
  • \$\begingroup\$ I think the recommendation to use a print of \n over std::endl merits a bit more explanation - is flushing always bad? \$\endgroup\$
    – ades
    Jan 5 at 12:12
  • \$\begingroup\$ No worries @ades the flushing part I don't mind for this example. this is very useful thank you \$\endgroup\$ Jan 5 at 12:30
  • \$\begingroup\$ Could you provide a link/source for how that equality 1 liner works, I have no understanding of why we can simply do const = default \$\endgroup\$ Jan 5 at 12:37
  • \$\begingroup\$ This doesn't compile on clion due to there not being a constructor, why is not using the default constructor? \$\endgroup\$ Jan 5 at 12:40
  • \$\begingroup\$ @AlexisDrakopoulos, have you set your compiler to accept a sufficiently modern version of C++? It's a good idea to select the most recent version it can support; e.g. I generally use g++ -std=c++20 when compiling with GCC. \$\endgroup\$ Jan 5 at 13:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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