1
\$\begingroup\$

I implemented a vector class in C for integers. Any feedback welcome.

header:

#define DEFAULT_CAPACITY 1000

// Define a vector type
typedef struct 
{
  int nrOfElements;     // Current number of elements.
  int capacity; // Possible capacity.  
  int *data;    // Pointer to array elements.    
} CVectorInt;

typedef enum {VECTOR_SUCCESS, VECTOR_OUTOFBOUNDS, VECTOR_MEMORYERROR} vector_error;

vector_error CVectorInit(CVectorInt *vector);
vector_error CVectorAppend(CVectorInt *vector, int value);
vector_error CVectorGet(CVectorInt *vector, int index, int * value);
vector_error CVectorSet(CVectorInt *vector, int index, int value);
void CVectorFree(CVectorInt *vector);

.c:

#include "CVectorInt.h"
#include <stdio.h>
#include <stdlib.h>


vector_error CVectorInit(CVectorInt *vector)
{
    int i = 0;

    vector->capacity = DEFAULT_CAPACITY;
    vector->nrOfElements = 0;
    vector->data = malloc(sizeof(int) * vector->capacity);
    if(vector->data == NULL)
        return VECTOR_MEMORYERROR;

    // Initialization in C never hurts.
    for(i = 0; i<DEFAULT_CAPACITY; i++)
        vector->data[i] = 0;

    return VECTOR_SUCCESS;
}

vector_error CVectorAppend(CVectorInt *vector, int value) 
{
    int * ptr = NULL;
    int i = 0;

    // Resize vector if there is no space for more elements.
    if(vector->capacity == vector->nrOfElements)
    {
        vector->capacity *= 2;
        ptr = realloc(vector->data, vector->capacity * sizeof(int));
        if(ptr == NULL)          
        {       
            return VECTOR_MEMORYERROR;
        }
        else
        {
            vector->data = ptr; 

            // Initialize new elements to 0.
            for(i = vector->nrOfElements; i<vector->capacity; i++)
                vector->data[i] = 0;
        }
    }

    vector->data[vector->nrOfElements++] = value;

    return VECTOR_SUCCESS;


}

vector_error CVectorGet(CVectorInt *vector, int index, int * value)
{
    if(index < vector->nrOfElements && index >= 0)
    {
        // Pass value at given index.
        *value = vector->data[index];
        return VECTOR_SUCCESS;
    }else
    {
        return VECTOR_OUTOFBOUNDS;
    }
}

vector_error CVectorSet(CVectorInt *vector, int index, int value)
{
    if(index >= vector->capacity)
        return VECTOR_OUTOFBOUNDS;

    vector->data[index] = value;

    return VECTOR_SUCCESS;

}


void CVectorFree(CVectorInt *vector)
{
  free(vector->data);
}

Usage:

#include <stdio.h>
#include <stdlib.h>
#include "CVectorInt.h"

int main(void) 
{
    //error handling omitted for simplicity

    CVectorInt v;
    int i = 0, temp = 0;

    CVectorInit(&v);
    for(i = 0; i<100; i++)
    {
        CVectorAppend(&v, i*2);
    }

    for(i = 0; i<100; i++)
    {
        CVectorGet(&v, i, &temp);
        printf("%d ", temp);
    }

    CVectorFree(&v);

    return 0;
}
\$\endgroup\$
10
  • \$\begingroup\$ Have you handled overflow errors appropriately? \$\endgroup\$
    – Pimgd
    Mar 15 '16 at 8:35
  • \$\begingroup\$ @Pimgd what kind of overflow errors are you referring to \$\endgroup\$
    – user100282
    Mar 15 '16 at 8:40
  • 1
    \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$
    – Mast
    Mar 15 '16 at 13:31
  • 3
    \$\begingroup\$ @User100 I recommend you go with a follow up question, you got answers here, apply the changes, apply your own changes, as a new question, mark an answer here as accepted and ignore this one. \$\endgroup\$
    – Pimgd
    Mar 15 '16 at 14:06
  • 1
    \$\begingroup\$ @User100 If you disagree with any decision a user made, take it to Code Review Meta or find us in The 2nd Monitor. \$\endgroup\$
    – Mast
    Mar 15 '16 at 14:32
2
\$\begingroup\$

I think you should use uint for things where you cannot have negative values.

vector_error CVectorGet(CVectorInt *vector, int index, int * value)
{
    if(index < vector->nrOfElements && index >= 0)

This check, for instance, wouldn't need the comparison with 0 if you used uint.

In the comments I already said something about overflow. Now, you can indeed say "that's such a big amount of elements, people are not going to surpass this" and not build it in. That's okay. Be sure to add it in as a comment in the header though, because you don't want to accidentally find out that this is a problem.

\$\endgroup\$
1
2
\$\begingroup\$

Some points:

  • CVectorSet: what should happen if index > vector->nrOfElements? Should nrOfElements=index?
  • The vector_error enum should be rename as vector_status, I think.
  • vector->capacity *= 2; This is a geometric progression, it may grows faster then the user think. You may want to use an arithmetic progression (vector->capacity += DEFAULT_CAPACITY; for example).
  • vector->capacity *= 2; You should fix a limit to the capacity.
  • if(index >= vector->capacity) return VECTOR_OUTOFBOUNDS; Why not checking the lower bound too? (Or use unsigned int.)
  • Since you're initializing the space to zero, you should use calloc instead of malloc.
\$\endgroup\$
2
  • \$\begingroup\$ "Should nrOfElements=index?" no I just increase nrOfElements \$\endgroup\$
    – user100282
    Mar 15 '16 at 13:36
  • \$\begingroup\$ if nrOfElements>index then I don't increase nrOfElements \$\endgroup\$
    – user100282
    Mar 15 '16 at 13:43

Your Answer

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