Managing initialization for a vector abstract data type in C

Question

As an assignment, I was to create an \$n\$-dimensional vector type for \$n \le 20\$. This vector only has to support addition and scalar multiplication.

I tried to write good code, with proper memory management, error reporting, and a nice API. However, I have little experience with C so I would welcome any feedback on my code, especially on whether the initialization and error handling workflow could be made more idiomatic. This is how my vector is supposed to be used:

Vector v = Vector_EMPTY;
Vector_E err_new = Vector_new(&v, 3);
if (err_new) {
    ... // handle error
}

// write the values into the initialized vector
double values[] = {1, 2, 3};
for (size_t i = 0; i < 3; i++) {
    v.at[i] = values[i];
}

// twice_v = v + v
Vector twice_v = Vector_EMPTY;
Vector_E err_add = Vector_add(v, v, &twice_v);
if (err_add) {
    ... // handle error
}

... // do something useful

Vector_free(&twice_v);
Vector_free(&v);

And this is the Vector implementation:

#include <stdbool.h>
#include <stdlib.h>

/// A n-dimensional vector type
///
///     Vector v = Vector_EMPTY;
///     Vector_E result_new = Vector_new(&v, dimensions);
///     ...
///     Vector_free(&v);
///
/// To access a dimension by index, use the `.at` field:
///
///     v.at[3] // the 4th dimension
typedef struct {
    double *at;
    size_t size;
    bool valid;
} Vector;

#define Vector_EMPTY (Vector){NULL, 0, false};

typedef enum {
    Vector_E_SUCCESS,
    Vector_E_ALLREADY_INITIALIZED,
    Vector_E_MALLOC,
    Vector_E_DIMENSION
} Vector_E;

const char *
Vector_E_get_name(Vector_E status) {
    switch (status) {
        case Vector_E_SUCCESS:
            return "Vector_E_SUCCESS";
        case Vector_E_ALLREADY_INITIALIZED:
            return "Vector_E_ALLREADY_INITIALIZED";
        case Vector_E_MALLOC:
            return "Vector_E_MALLOC";
        case Vector_E_DIMENSION:
            return "Vector_E_DIMENSION";
        default:
            return "Vector_E_UNKNOWN";
    }
}

/// Initialize a new Vector that has been allocated outside:
///
///     Vector v = Vector_EMPTY;
///     Vector_E result = Vector_new(&v, 42);
///
/// If `Vector_new` was successful, you must call `Vector_free` to properly release memory!
///
/// Returns the following status codes: SUCCESS, ALLREADY_INITIALIZED, MALLOC
Vector_E
Vector_new(Vector *v, size_t n) {
    if (v->valid) {
        return Vector_E_ALLREADY_INITIALIZED;
    }
    double *values = calloc(n, sizeof(double));
    if (!values) {
        return Vector_E_MALLOC;
    }
    v->size = n;
    v->at = values;
    v->valid = true;
    return Vector_E_SUCCESS;
}

/// Prepares the vector for deallocation
void
Vector_free(Vector *v) {
    v->valid = false;
    free(v->at);
}

/// Adds to vectors a and b into result, which must be uninitialized.
///
///     Vector result = Vector_EMPTY;
///     Vector_E result_add = Vector_add(a, b, &result);
///     ...
///     Vector_free(result);
///
/// Returns all status codes that `Vector_new` does, and DIMENSION if the dimensions of the input vectors don't match.
Vector_E
Vector_add(Vector a, Vector b, Vector *result) {
    // both vectors must have same dimensionality
    if (a.size != b.size) {
        return Vector_E_DIMENSION;
    }
    size_t size = a.size;
    Vector_E status_new = Vector_new(result, size);
    if (status_new != Vector_E_SUCCESS) {
        return status_new;
    }
    for (size_t i = 0; i < size; i++) {
        result->at[i] = a.at[i] + b.at[i];
    }
    return Vector_E_SUCCESS;
}

/// Calculates the scalar product of two vectors a and b and writes the result into `result`.
///
///     double result;
///     Vector_E status = Vector_scalar_product(a, b, &result);
///
/// Returns the DIMENSION status code if the input vectors have different dimensions, otherwise SUCCESS.
Vector_E
Vector_scalar_product(Vector a, Vector b, double *result) {
    if (a.size != b.size) {
        return Vector_E_DIMENSION;
    }
    size_t size = a.size;
    double my_result = 0;
    for (size_t i = 0; i < size; i++) {
        my_result += a.at[i] * b.at[i];
    }
    *result = my_result;
    return Vector_E_SUCCESS;
}

Answer 1

For the most part, this is pretty solid. I only have one major gripe, and that's this:

#define Vector_EMPTY (Vector){NULL, 0, false};

(It also shouldn't have a semicolon at the end of the line, but that's a minor point.)

Why are you initializing your vectors this way? This is silly for a couple of reasons.

1. It just makes your code more complicated. It forces you to create new vectors using Vector_EMPTY (which really isn't clearly named, anyway), then pass them around to functions by reference.
2. It adds some unnecessary mutation where no mutation should be required, making your code harder to reason about.

New proposal—just return Vector objects. Make the errors the "out" parameters, not the vectors. This changes your function signatures to the following:

Vector Vector_new(size_t n, Vector_E* error);
Vector Vector_add(Vector a, Vector b, Vector_E* error);
double Vector_scalar_product(Vector a, Vector b, Vector_E* error);

The primary result of a function should be its return value. Use "out" parameters for extras, like errors.

What if malloc fails, then? Well, frankly, if malloc fails, you've probably got bigger problems, so just return a Vector with NULL for the at field.

This change allows you to eliminate the valid field as well.

---

I'd change the signature of Vector_free to the following:

void Vector_free(Vector v);

You pass around vectors in other functions by value, and I think it makes more sense to pass them into free by value as well. The structures themselves are tiny, so you're probably getting more performance by copying instead of dereferencing a pointer. Either way, it's clearer, and therefore, it's preferred.

---

That's it! Otherwise, your code looks clear and well-written. Nicely done.