HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Template 3d vector class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
templatevectorclass

Problem

I've created this 3D vector class and would like it to be reviewed. I wonder if there are still some improvements that can be made. This is a header only implementation.

```
#pragma once
#include

template class vec3
{
public:
union {
T f[3];

struct {
T x, y, z;
};
};

vec3(vec2 p, T z = 0) :
x(p.x),
y(p.y),
z(z)
{
}
vec3(T x = 0, T y = 0, T z = 0) :
x(x),
y(y),
z(z)
{
}

template operator vec2() const { return vec3(x, y); };
template operator vec3() const { return vec3(x, y, z); };

vec3 operator-() const {
return vec3(-x, -y, -z);
};

bool operator==(const vec3& other) const {
return (x == other.x && y == other.y && z == other.z);
};
bool operator!=(const vec3& other) const {
return !operator==(other);
};

bool operator other.x)
return false;
//x == other.x
if (y other.y)
return false;
//y == other.y
if (z other.z)
return false;
//z == other.z
return false;
};
bool operator>(const vec3& other) const {
return other (other);
};
bool operator>=(const vec3& other) const {
return !operator(x other.x, y other.y, z * other.z);
};

vec3& operator/= (const vec3& other) {
x /= other.x;
y /= other.y;
z /= other.z;
return *this;
};
vec3 operator/ (const vec3& other) const {
return vec3(x / other.x, y / other.y, z / other.z);
};

vec3& operator+= (const vec3& other) {
x += other.x;
y += other.y;
z += other.z;
return *this;
};
vec3 operator+ (const vec3& other) const {
return vec3(x + other.x, y + other.y, z + other.z);
};

vec3& operator-= (const vec3& other) {
x -= other.x;
y -= other.y;
z -= other.z;
return *this;
};

Solution

I see a number of things that may help you improve your program.

Include all required headers

The template has calls to acos and sin but doesn't include the ` header which is required. The code should have this line:

#include 


Use namespaces

The previously mentioned
acos and sin should use the namespace prefix as std::acos and std::sin.

Don't use anonymous
structs

While they are standard in C11, they are not allowed in C++. There are compiler extensions that can enable them but they don't conform to the standard. In this case, you could either name it or use the next suggestion.

Eliminate the
union

Within the array is never used. This suggests that the union could simply be eliminated. If you think that users of the code are going to want to refer to the elements as an array, you could provide an
operator[] implementation for that with cleaner syntax anyway.

Eliminate
vec2

The
vec3 class does not seem to rely on a vec2 for existence or functionality, so it should not be a part of the class definition. Right now, it's not possible to use the class unless one has a vec2 also available, but that dependency is also not #included per the first suggestion.

Don't use
#pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions. By definition, all
#pragma are non-standard. For portable code, you should use the standard include guards. Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Think about
unsigned numbers

It may be that you intended for this class to use exclusively floating point numbers, but there isn't any checking of that, so the result is that this line:

std::cout ::left.toString() << '\n';


compiles and runs just fine, but gives a peculiar result:

[x: 4294967295, y: 0, z: 0]


I would recommend adding a
static_assert to the class:

static_assert(std::is_signed{}, "Error: vec3 type must be signed");


Use appropriate floating point types

If I am using a
vec3, I might not be happy that angle is internally using only a float instead. Consider using either T or double where float is currently used within the class member functions.

Wrap it in a namespace

To make sure your
vec3` doesn't collide with another in some other library, you might want to wrap the whole thing up in your own namespace.

Code Snippets

#include <cmath>
std::cout << vec3<unsigned>::left.toString() << '\n';
[x: 4294967295, y: 0, z: 0]
static_assert(std::is_signed<T>{}, "Error: vec3 type must be signed");

Context

StackExchange Code Review Q#123744, answer score: 3

Revisions (0)

No revisions yet.