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

vector3 math module

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

Problem

I'm writing a little math module in C to handle vectors and matrices. This will be column-major style but right now I've only finished the basics of the vector functions and wanted some feedback on optimization tips, naming conventions, etc. This is C so no real function overloading. I would like to keep it C style.

vec3f.h

#include "stdlib.h"
#include "math.h"

const double m_PI = 3.14159265359;

typedef struct vec3f
{
    double p[3];
}vec3f;

vec3f* vec3fx(double x, double y, double z);
vec3f* vec3fv(vec3f* v3);
vec3f  vec3fs(double x, double y, double z);
void vec3fadd(vec3f* out, struct vec3f* a, struct vec3f* b);
void vec3fsub(vec3f* out, struct vec3f* a, struct vec3f* b);
void vec3fmul(vec3f* out, struct vec3f* a, struct vec3f* b);

void vec3fnegate(vec3f* out);

void vec3fscale(vec3f* out, double a);
void vec3fscaleu(vec3f* out, double x, double y, double z);
void vec3fscalex(vec3f* out, double a);
void vec3fscaley(vec3f* out, double a);
void vec3fscalez(vec3f* out, double a);

void vec3flen(double* out, vec3f* in);

void toDegrees(double* out, double angleInRadians);
void toRadians(double* out, double angleInDegrees);

void vec3fdot(double* out, vec3f* a, vec3f* b);
void vec3fcross(vec3f* out, vec3f* a, vec3f* b);

void vec3fnorm(vec3f* out, vec3f* in);
void vec3fdist(double* out, vec3f* a, vec3f* b);

void vec3fangle(double* out, vec3f* a, vec3f* b);


vec3f.c

```
#include "vec3f.h"

vec3f* vec3fx(double x, double y, double z)
{
vec3f v = (vec3f)malloc(sizeof(vec3f));
v->p[0] = x;
v->p[1] = y;
v->p[2] = z;
return v;
}

vec3f vec3fv(vec3f v3)
{
vec3f v = (vec3f)malloc(sizeof(vec3f));
v->p[0] = v3->p[0];
v->p[1] = v3->p[1];
v->p[2] = v3->p[2];
return v;
}

void vec3fadd(vec3f out, vec3f a, vec3f* b)
{
out->p[0] = a->p[0] + b->p[0];
out->p[1] = a->p[1] + b->p[1];
out->p[2] = a->p[2] + b->p[2];
}

void vec3fsub(vec3f out, vec3f a, vec3f* b)
{
out->p[0] = a->p[0] - b->p[0];
out->p[1

Solution

This seems to be generally well-written code. I have a few observations which may help you improve your code.

Separate interface from implementation

The interface is the part in the vec3f.h file and the implementation is in the vec3f.c file. Users of this code should be able to read and understand everything they need from the implementation file. That means that only #includes essential to being able to understand the interface should be in the .h file. In this case, none are needed, so the #include lines should be deleted from the .h file. Also...

Use all appropriate #includes

The implementation file (the .c file) does need some #include files but they should be in angle brackets and not quotes. When you write #include "math.h" it is different from #include . For standard headers, you should use the <> form. If you're not sure about the difference, see this question for more detail. In this case, vec3f.c should start with:

#include "vec3f.h"
#include 
#include 


Use include guards

There should be an include guard in the .h file. That is, start the file with:

#ifndef VEC3F_H
#define VEC3F_H
// file contents go here
#endif // VEC3F_H


Omit spurious declarations

The vec3f.h file includes a declaration for vec3fs but no implementation exists in the vec3f.c file. The declaration should be omitted.

Use const where practical

For a number of the operations, the vec3f structures passed in are unaltered. That's good design, but you should make it explicit. So for example instead of this:

void vec3fadd(vec3f* out, struct vec3f* a, struct vec3f* b);


Use this instead:

void vec3fadd(vec3f* out, const vec3f* a, const vec3f* b);


Note that this change also uses the next suggestion.

Use defined typedef where practical

The declarations for vec3fadd, vec3fsub and vec3fmul should use the the typedef'd vec3f rather than the longer struct vec3f. Both are correct, but since you've created the typedef, it makes sense to also use it consistently.

Provide a means for non-heap allocation

Perhaps your unwritten vec3fs function was intended to do this, but there is currently no way to initialize values for an stack-based vec3f. I'd recommend adding one like this:

vec3f* vec3fs(vec3f* v, double x, double y, double z)
{
    if (v != NULL) {
        v->p[0] = x;
        v->p[1] = y;
        v->p[2] = z;
    }
    return v;
}


Check the return value of malloc

If the program runs out of memory, a call to malloc can fail. The indication for this is that the call will return a NULL pointer. You should check for this and avoid dereferencing a NULL pointer (which typically causes a program crash). For example, using the vec3fs above, you could rewrite vec3fx as:

vec3f* vec3fx(double x, double y, double z)
{
    vec3f* v = malloc(sizeof(vec3f));
    return vec3fs(v, x, y, z);
}


Note also that the cast is not required since malloc returns a void *.

Don't pointlessly initialize variables

The current code includes this function:

void vec3fdot(double* out, vec3f* a, vec3f* b)
{
    double total = 0;
    total += a->p[0] * b->p[0];
    total += a->p[1] * b->p[1];
    total += a->p[2] * b->p[2];
    *out = total;
}


Consider instead this minor rewrite of vec3fdot:

double vec3fdot(const vec3f* a, const vec3f* b)
{
    return a->p[0] * b->p[0]
         + a->p[1] * b->p[1]
         + a->p[2] * b->p[2];
}


Return something useful from functions

The way the functions are currently written, most return void but this prevents one from writing useful construct such as:

toDegrees(vec3fangle(vec1, vec2));


To support this, you could rewrite those two functions as:

double toDegrees(double angleInRadians)
{
    return angleInRadians * (double)(180.0/m_PI);
}
double vec3fangle(const vec3f* a, const vec3f* b)
{
    return acos(vec3fdot(a, b)/(vec3flen(a)*vec3flen(b)));
}


Similarly, I'd recommend that each function that takes a vec3f *out as the first parameter should also return it. This would make your vec3f.h file look like this:

vec3f.h

```
#ifndef VEC3F_H
#define VEC3F_H
const double m_PI = 3.14159265359;

typedef struct vec3f
{
double p[3];
}vec3f;

vec3f* vec3fx(double x, double y, double z);
vec3f vec3fv(vec3f v3);
vec3f vec3fs(vec3f v, double x, double y, double z);
vec3f vec3fadd(vec3f out, const vec3f a, const vec3f b);
vec3f vec3fsub(vec3f out, const vec3f a, const vec3f b);
vec3f vec3fmul(vec3f out, const vec3f a, const vec3f b);

vec3f vec3fnegate(vec3f out);

vec3f vec3fscale(vec3f out, double a);
vec3f vec3fscaleu(vec3f out, double x, double y, double z);
vec3f vec3fscalex(vec3f out, double a);
vec3f vec3fscaley(vec3f out, double a);
vec3f vec3fscalez(vec3f out, double a);

double vec3flen(const vec3f* in);

double toDegrees(double angleInRadians);
double toRadians(double angleInDegrees);

double vec3fdot(const

Code Snippets

#include "vec3f.h"
#include <stdlib.h>
#include <math.h>
#ifndef VEC3F_H
#define VEC3F_H
// file contents go here
#endif // VEC3F_H
void vec3fadd(vec3f* out, struct vec3f* a, struct vec3f* b);
void vec3fadd(vec3f* out, const vec3f* a, const vec3f* b);
vec3f* vec3fs(vec3f* v, double x, double y, double z)
{
    if (v != NULL) {
        v->p[0] = x;
        v->p[1] = y;
        v->p[2] = z;
    }
    return v;
}

Context

StackExchange Code Review Q#98313, answer score: 6

Revisions (0)

No revisions yet.