3
\$\begingroup\$

This is follow up to this one. In the previous some minor errors slipped, so I couldn't update the code. Also took some advice. So final version is below.

header:

#define DEFAULT_CAPACITY 1000

// Define a vector type
typedef struct 
{
  int nrOfElements; // This just denotes current number of elements.
  int capacity;     // This denotes the capacity of the array.
  int *data;        // Actual pointer to array elements.    
} CVectorInt;

typedef enum {VECTOR_SUCCESS, VECTOR_OUTOFBOUNDS, VECTOR_MEMORYERROR, VECTOR_LIMITEXCEEDED} vector_status;

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

source:

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

///
// Initialize vector.
//  Set capacity to default value. Current number of elements to 0.
//  Allocate memory dynamically for the default number of elements.
//
//
// Return
//  If there is some problem, error code is returned.
//
vector_status CVectorInit(CVectorInt *vector)
{
    int i = 0;

    vector->capacity = DEFAULT_CAPACITY;
    vector->nrOfElements = 0;

    // Allocate array
    vector->data = malloc(sizeof(int) * vector->capacity);
    if(vector->data == NULL)
        return VECTOR_MEMORYERROR;

    // Initialization the array to default values.
    for(i = 0; i < DEFAULT_CAPACITY; i++)
        vector->data[i] = 0;

    return VECTOR_SUCCESS;
}

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

    if(vector->capacity == 32000)
        return VECTOR_LIMITEXCEEDED;

    // Do we have room to append elements?
    if(vector->capacity <= vector->nrOfElements)
    {
        // No, increase capacity
        vector->capacity *= 2;

        // Resize array
        ptr = realloc(vector->data, vector->capacity * sizeof(int));
        if(ptr == NULL)          
        {       
            // We had an error
            return VECTOR_MEMORYERROR;
        }
        else
        {
            vector->data = ptr; 

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

    // Now, after possible resize, add the element.
    vector->data[vector->nrOfElements++] = value;

    return VECTOR_SUCCESS;


}

vector_status CVectorGet(CVectorInt *vector, int index, int * value)
{
    // Is the index within bounds?
    if(index < vector->nrOfElements && index >= 0)
    {
        *value = vector->data[index];
        return VECTOR_SUCCESS;
    }else
    {
        return VECTOR_OUTOFBOUNDS;
    }
}

vector_status CVectorSet(CVectorInt *vector, int index, int value)
{
    // We can't set outside capacity
    if(index >= vector->capacity || index < 0)
        return VECTOR_OUTOFBOUNDS;

    // zero fill the vector up to the desired index
    while(vector->nrOfElements <= index)
        CVectorAppend(vector, 0);

    vector->data[index]=value;
}


void CVectorFree(CVectorInt *vector)
{
  // Free underlying array
  free(vector->data);
}
\$\endgroup\$
2
\$\begingroup\$
  1. DEFAULT_CAPACITY has little value. Simple make the the size zero on initialization. The first time more memory is needed, then begin. Consider if code used many sparsely populated CVectorInt then that could tie up lots of unused memory. Rather see a much smaller initial size.

  2. CVectorFree() fails to update the other fields.

    void CVectorFree(CVectorInt *vector) {
      free(vector->data);
      vector->data = NULL;     
      vector->nrOfElements = 0;
      vector->capacity = 0;
    }
    
  3. I have found it useful to know the range of an enumerated type by adding the last one as _N

    typedef enum {
      VECTOR_SUCCESS, 
      VECTOR_OUTOFBOUNDS,
       ...
      VECTOR_LIMITEXCEEDED,
      VECTOR_N
      } vector_status;
    
    // sample usage
    const char *vector_status_message[VECTOR_N] =  { ... }
    

See little value with 32000 as an upper bound - it is arbitrary and surprising. If anything use, (SIZE_MAX/sizeof (int)) as the upper bound.

  1. Array sizes are best typed as size_t as that is the type return by sizeof() and work for indexing all arrays. Otherwise, why use int? It may be too narrow (once 32,000 is eliminated).

      size_t nrOfElements;
      size_t capacity;
    
  2. Missing functionality - CVectorInit_RightSize(). Assume this routine set became very useful. As code uses many array, some arrays may be stable and so a re-allocation based on the size used would be memory efficient.

  3. Naming: vector_status and CVector are too distinct. I'd expect the errors to be CVector_status or Vector_status (if used with other sets)

  4. Rather than taking the sizeof the type, take the size of the object. Less maintenance should code change form int to long, etc.

    // vector->data = malloc(sizeof(int) * vector->capacity);
    vector->data = malloc(sizeof *(vector->data) * vector->capacity);
    
  5. Missing file names. The name of the files is important to. Recommend CVector.c and CVector.h.

  6. Incorrect initialization. Also in other functions, code does not update fields in error conditions.

    vector_status CVectorInit(CVectorInt *vector)  {
      ...
      vector->capacity = DEFAULT_CAPACITY;
      ...
      vector->data = malloc(sizeof(int) * vector->capacity);
      if(vector->data == NULL) {
        vector->capacity = 0;  // <-- Missing 
        return VECTOR_MEMORYERROR;
      }
    
  7. I have found a sample usage of the routine (maybe in the header file) surrounded with #if 0 .... #endif a useful asset.

\$\endgroup\$
10
  • \$\begingroup\$ as far as I can see most of the comments here are about improvements, there are no serious flaws in the code \$\endgroup\$
    – user100282
    Mar 16 '16 at 7:52
  • \$\begingroup\$ @User100 Sorry if not clear. #2 & 9 are flaws that should be remedied. IMO #1 points out a design flaw. \$\endgroup\$ Mar 16 '16 at 10:39
  • \$\begingroup\$ :for me it makes more sense that vector->capacity is always default value(which gets only increased if there is need), no need to set it to 0 \$\endgroup\$
    – user100282
    Mar 16 '16 at 11:03
  • \$\begingroup\$ plus I don't see relevancy of 9 and 2; users should not use vector after free anyway \$\endgroup\$
    – user100282
    Mar 16 '16 at 11:12
  • \$\begingroup\$ @User100 #9 does not refer to use after free, but use after error. "users should not use vector after free" Your code lacks any prohibition concerning that. If code has restricted sequences of use, that should be made explicit in the code (preferable in the header file) rather than assume user has same knowledge of code as the creator.. Robust code assumes novel use. The more successful the code, the more it will be used in unforeseen ways. Adding the recommended code like vector->data = NULL; is trivial on your part yet prevents UB and aids in debugging. \$\endgroup\$ Mar 16 '16 at 12:59
2
\$\begingroup\$

Your entire CVectorInit function can essentially be trimmed down to this:

vector_status CVectorInit(CVectorInt *vector)
{
    vector->capacity = DEFAULT_CAPACITY;

    // Allocate array
    vector->data = calloc(vector->capacity, sizeof(int));
    if(vector->data == NULL)
        return VECTOR_MEMORYERROR;

    return VECTOR_SUCCESS;
}

Fundamentally though, is there a reason to zero out the allocated array? You make sure that you never access an index that's out of bounds in your functions so there should be no need to zero out the allocated array. Especially on resizing the vector, this will just be a performance hit that you'll take with little benefit (especially when the vector gets to be quite large).

Moreover, I would replace the ints holding the size and capacity of the vector with size_t or something similar. These values are never going to hold negative numbers so there's no need to have them as signed ints.

Also, there seems to be this magic constant 32000. Why is it 32k and what's the reasoning? Create a named variable for this constant. So in your .c file:

static const size_t MAXIMUM_CAPACITY = 32000;

Also, why is that value so low? A standard signed int can hold a maximum value of around 2 billion so 32k seems pretty low. Maybe putting a comment above that named variable to justify your reason could be helpful.

As a design consideration, you might want to think whether or not you want to have DEFAULT_CAPACITY declared as a macro in your header file. As it is, someone could go to your header and set DEFAULT_CAPACITY to a negative value or some other erroneous number. Assuming that you might be compiling this as some sort of library, it might be better to declare DEFAULT_CAPACITY as a private variable to your implementation file to avoid such things:

In your .c file: static const size_t DEFAULT_CAPACITY = 1000;

This also comes with the added benefit of type safety and avoiding the use of the preprocessor.

\$\endgroup\$
1
\$\begingroup\$
if(vector->capacity == 32000)
    return VECTOR_LIMITEXCEEDED;

Just like you define DEFAULT_CAPACITY, you should try to define MAX_CAPACITY in your header.

Additionally, it would be better to check with >=, because if something happens and default is lowered to 100, then you get 100, 200, 400, 800, 1600, 3200, 6400, 12800, 25600... and then 51200, which is over the limit of 32000, but because you're using == 32000, it doesn't get detected. With >=, it would be.


}else
{
    return VECTOR_OUTOFBOUNDS;
}

Minor formatting error? You have

}
else
{

in other places, so it looks like you missed a newline.


// Initialization the array to default values.
for(i = 0; i < DEFAULT_CAPACITY; i++)
    vector->data[i] = 0;

and

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

It seems there's a use for a private method fill(CVectorInt *vector, int startIndex, int endIndex, int value).


if(index < vector->nrOfElements && index >= 0)

For these sort of conditionals, I prefer to write them as a number range:

if(0 <= index && index < vector->nrOfElements)

That way, it reads like "if index is between 0 and vector->nrOfElements".

I would even rename nrOfElements to elementCount, but that's another matter of opinion.

\$\endgroup\$
2
  • \$\begingroup\$ what's the formatting error? \$\endgroup\$
    – user100282
    Mar 15 '16 at 14:40
  • \$\begingroup\$ @User100 updated answer \$\endgroup\$
    – Pimgd
    Mar 15 '16 at 14:42
1
\$\begingroup\$

Other hints:

  • In your #include list, local headers should appear after system headers. This is both a matter of style convention and a safeguard against your headers interfering with system headers - in C, they are not isolated. In C++, they are only isolated if they properly use namespaces.

  • Use #ifdef guards in your header to prevent double inclusion, or if your compiler supports it (as nearly all compilers do), #pragma once.

  • Proper loose coupling should require that the structure is declared as opaque in the header and defined fully only in your C file. That way, other users of your library will not be allowed to inappropriately directly manipulate the structure's members, and can only manipulate them via library function calls. There are some good examples here: https://stackoverflow.com/questions/3965279/opaque-c-structs-how-should-they-be-declared

\$\endgroup\$
2
  • \$\begingroup\$ Can you please throw example of how to apply opaque in this cide? I am getting some errors on compile. \$\endgroup\$
    – user100282
    Mar 16 '16 at 11:04
  • \$\begingroup\$ @User100 Alright - link added. \$\endgroup\$
    – Reinderien
    Mar 16 '16 at 11:49

Your Answer

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