current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -1 down vote favorite

I don't really like SFML's Vector Classes so I tried making my own. Any criticism is welcome.

    class gVector2{
    public:
        float x, y;
        float length = GetLength();
        gVector2();
        gVector2(float X, float Y) : x(X), y(Y){};
        float GetLength(){ return sqrtf((x * x) + (y * y)); }
        void Normalize(){ x /= length, y /= length; }
        float Dot(gVector2 rightVec){ return (x * rightVec.x) + (y * rightVec.y); }

        gVector2 operator+ (gVector2);
        gVector2 operator- (gVector2);
        gVector2 operator* (float);
        gVector2 operator/ (float);
        void operator+= (gVector2);
        void operator-= (gVector2);
        void operator*= (float);
        void operator/= (float);
        void operator= (gVector2);
        bool operator== (gVector2);
        bool operator!= (gVector2);
};

gVector2::gVector2(){
    x = 0;
    y = 0;
}

gVector2 gVector2::operator+(gVector2 rightVec){
    gVector2 solution;
    solution.x = x + rightVec.x;
    solution.y = y + rightVec.y;
    return solution;
}

gVector2 gVector2::operator-(gVector2 rightVec){
    gVector2 solution;
    solution.x = x - rightVec.x;
    solution.y = y - rightVec.y;
    return solution;
}

gVector2 gVector2::operator*(float scalar){
    gVector2 solution;
    solution.x = x * scalar;
    solution.y = y * scalar;
    return solution;
}

gVector2 gVector2::operator/(float scalar){
    gVector2 solution;
    solution.x = x / scalar;
    solution.y = y / scalar;
    return solution;
}

void gVector2::operator+=(gVector2 rightVec){
    x += rightVec.x;
    y += rightVec.y;
}

void gVector2::operator-=(gVector2 rightVec){
    x -= rightVec.x;
    y -= rightVec.y;
}

void gVector2::operator*=(float scalar){
    x *= scalar;
    y *= scalar;
}

void gVector2::operator/=(float scalar){
    x /= scalar;
    y /= scalar;
}

void gVector2::operator=(gVector2 rightVec){
    x = rightVec.x;
    y = rightVec.y;
}

bool gVector2::operator==(gVector2 rightVec){
    if (x == rightVec.x &&
        y == rightVec.y)
    {
        return true;
    }
    else{
        return false;
    }
}

bool gVector2::operator!=(gVector2 rightVec){
    if (x != rightVec.x ||
        y != rightVec.y)
    {
        return true;
    }
    else{
        return false;
    }
}

class gVector3{
public:
    float x, y, z;
    float length = GetLength();
    gVector3();
    gVector3(float X, float Y, float Z) : x(X), y(Y), z(Z) {};

    float GetLength(){ return sqrtf((x * x) + (y * y) + (z * z)); }
    void Normalize(){ x /= length, y /= length, z /= length; }
    float Dot(gVector3 rightVec){ return (x * rightVec.x) + (y * rightVec.y) + (z * rightVec.z); }
    gVector3 Cross(gVector3);

    gVector3 operator+ (gVector3);
    gVector3 operator- (gVector3);
    gVector3 operator* (float);
    gVector3 operator/ (float);
    void operator+= (gVector3);
    void operator-= (gVector3);
    void operator*= (float);
    void operator/= (float);
    void operator= (gVector3);
    bool operator== (gVector3);
    bool operator!= (gVector3);


};

gVector3::gVector3(){
    x = 0;
    y = 0;
    z = 0;
}

gVector3 gVector3::Cross(gVector3 rightVec){
    gVector3 crossProduct;
    crossProduct.x = (y * rightVec.z) - (z * rightVec.y);
    crossProduct.y = (z * rightVec.x) - (x * rightVec.z);
    crossProduct.z = (x * rightVec.y) - (y * rightVec.x);
    return crossProduct;
}

gVector3 gVector3::operator+(gVector3 rightVec){
    gVector3 solution;
    solution.x = x + rightVec.x;
    solution.y = y + rightVec.y;
    solution.z = z + rightVec.z;
    return solution;
}

gVector3 gVector3::operator-(gVector3 rightVec){
    gVector3 solution;
    solution.x = x - rightVec.x;
    solution.y = y - rightVec.y;
    solution.z = z - rightVec.z;
    return solution;
}

gVector3 gVector3::operator*(float scalar){
    gVector3 solution;
    solution.x = x * scalar;
    solution.y = y * scalar;
    solution.z = z * scalar;
    return solution;
}

gVector3 gVector3::operator/(float scalar){
    gVector3 solution;
    solution.x = x / scalar;
    solution.y = y / scalar;
    solution.z = z / scalar;
    return solution;
}

void gVector3::operator+=(gVector3 rightVec){
    x += rightVec.x;
    y += rightVec.y;
    z += rightVec.z;
}

void gVector3::operator-=(gVector3 rightVec){
    x -= rightVec.x;
    y -= rightVec.y;
    z -= rightVec.z;
}

void gVector3::operator*=(float scalar){
    x *= scalar;
    y *= scalar;
    z *= scalar;
}

void gVector3::operator/=(float scalar){
    x /= scalar;
    y /= scalar;
    z /= scalar;
}

void gVector3::operator=(gVector3 rightVec){
    x = rightVec.x;
    y = rightVec.y;
    z = rightVec.z;
}

bool gVector3::operator==(gVector3 rightVec){
    if(x == rightVec.x &&
       y == rightVec.y &&
       z == rightVec.z)
    {return true;
    }else{
    return false;
    }
}

bool gVector3::operator!=(gVector3 rightVec){
    if (x != rightVec.x ||
        y != rightVec.y ||
        z != rightVec.z)
    {
        return true;
    }
    else{
        return false;
    }
}
share|improve this question
2  
I'll separate it into a header and cpp file when I have some more time to work on it. I would recommend waiting to post a question on SE when you have time. If a question requires feedback from you at the start, and you're not around, it may pose a problem. If you don't have time to help the reviewers, then they may not have enough time to help you. –  Jamal♦ Sep 14 '14 at 2:55
3  
Okay, but as you're new here, I should tell you that reviewers are allowed to comment on any aspect of the code. They may assume that this is how it's structured locally, and review on that anyway. –  Jamal♦ Sep 14 '14 at 3:03

1 Answer 1

up vote 5 down vote accepted

As it stands, your code has quite a few issues. Since you are familiar with SFML's vector classes, I suggest taking a look at the library to get some ideas from a well established codebase before starting your own. Also, see this question for a very similar Vector implementation review.

OK, onto the review:

Problems with the operators:

All these operators are incorrect:

void operator+= (gVector2);
void operator-= (gVector2);
void operator*= (float);
void operator/= (float);
void operator= (gVector2);

Same goes for gVector3. They were supposed to return a reference to the type you are manipulating. This allows composing expressions like you would do with an integer, for example. The correct signature is:

gVector2 & operator+= (gVector2);
gVector2 & operator-= (gVector2);
gVector2 & operator*= (float);
gVector2 & operator/= (float);
gVector2 & operator= (gVector2);

Returning *this in the implementation.

The comparison operators for both gVector2 and gVector3

bool operator== (gVector2);
bool operator!= (gVector2);

Should be made const, since they are not altering member state:

bool operator== (const gVector2 &) const;
bool operator!= (const gVector2 &) const;

Also, in this case, it is probably better to take the input parameter by const ref, instead of by value.

These should also be const:

gVector2 operator+ (gVector2) const;
gVector2 operator- (gVector2) const;
gVector2 operator* (float) const;
gVector2 operator/ (float) const;

But they return by value since each will produce a new vector, so this is correct.

float GetLength() const
float Dot(gVector2 rightVec) const;

Any member function that does not mutate class data should be make const. This is a good practice.

Storing length as a member:

This seems like a major design flaw to me. Once the vector is constructed it sets the length member, but as soon as someone changes any of the elements of the vector, length is no longer valid. You shouldn't keep that member variable at all. The GetLength() method is there so that the user can get a "fresh" length whenever is necessary.

BTW, gVector3::Normalize() will produce wrong results if length is not up-to-date.

Miscellaneous:

sqrtf() is not standard. Instead, include <cmath> and use std::sqrt().

In the implementation of all the operators, it would be a lot better and more efficient to construct the returned object in-place, instead of having those local variables:

gVector2 gVector2::operator - (gVector2 rightVec) 
{
    return gVector2(x - rightVec.x, y - rightVec.y);
}
// The same for all the others...

You have inconsistent/mixed { positioning on the ifs:

if (x == rightVec.x &&
    y == rightVec.y)
{
    return true;
}
else{
    return false;
}

Either use all like this:

if (x) {
    do_x();
}
else {
    do_y();
}

Or all like this:

if (x) 
{
    do_x();
}
else 
{
    do_y();
}

In the comparison operators, there is a much more concise way of doing the checks:

bool gVector2::operator == (const gVector2 & rightVec) const 
{
    return (x == rightVec.x && y == rightVec.y);    
}

No need for the if chains at all!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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