1
\$\begingroup\$

I have written a ArrayList class in C and I just wanted some criticism on what I can improve on. Any criticism helps, and I was wondering if there is any better way of doing error handling other than returning a enum.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <stdbool.h>
#include <errno.h>

#define ARRAY_LIST_DEFAULT_SIZE 5
#define RESIZE_MULTIPLIER 2

typedef struct arraylist_t
{
   void *array;
   size_t sizeOfElement;
   uint32_t sizeOfArray;
   uint32_t lengthOfArray;
   int32_t (*defaultCompare)(const void *, const void *);
   void (*defaultPrint)(const void *);
}ArrayList;

enum data_structure_error{
   E_SUCCESS = 0,
   E_FAIL = -1,
   E_MEMORY_FAIL = -2,
   E_VAR_UNINI = -3,
   E_VAR_TO_SMALL = -4,
   E_OUT_OF_BOUNDS = -5,
   E_PRINT_UNINI = -6,
   E_COMPARE_UNINI = -7
};

struct data_structure_errordesc{
    const int32_t  code;
    const char *message;
};

const struct data_structure_errordesc errordesc[] = {
   {E_SUCCESS, "No error" },
   {E_FAIL, "PANIC!!!!" },
   {E_MEMORY_FAIL, "Could not allocate memory"},
   {E_VAR_UNINI, "The variable passed in is uninitialized"},
   {E_VAR_TO_SMALL, "The variable passed in is too small to hold data"},
   {E_OUT_OF_BOUNDS, "Value passed in was out of bounds"},
   {E_PRINT_UNINI, "The default and the passed in print is NULL"},
   {E_COMPARE_UNINI, "The default and the passed in compare in NULL"}
};

void arraylist_error(enum data_structure_error error)
{
   uint32_t i;
   for(i = 0; i < sizeof(errordesc) / sizeof(errordesc[0]); i++)
   {
      if(error == errordesc[i].code)
      {
         printf("%s", errordesc[i].message);
         return;
      }
   }
}

enum data_structure_error arraylist_create(ArrayList ** const user_list, const size_t size_of_element, int32_t (*compare)(const void *, const void *), void (*print)(const void *))
{
   /*making 1 struct of type ArrayList*/
   ArrayList *list;

   if(user_list == NULL)
      goto FAIL0;

   list = malloc(sizeof(*list));
   if(list == NULL)
      goto FAIL1;

   /*making an array of void pointers*/
   list->array = malloc(size_of_element * ARRAY_LIST_DEFAULT_SIZE);
   if(list->array == NULL)
      goto FAIL1;

   /*setting all default values of ArrayList struct*/
   list->sizeOfElement = size_of_element;
   list->sizeOfArray = 0;
   list->lengthOfArray = ARRAY_LIST_DEFAULT_SIZE;
   list->defaultCompare = compare;
   list->defaultPrint = print;

   *user_list = list;
   return E_SUCCESS;

   FAIL0:
      return E_VAR_UNINI;
   FAIL1:
   free(list);
   return E_MEMORY_FAIL;
}

enum data_structure_error arraylist_clear(ArrayList * const list)
{
   if(list == NULL)
      return E_VAR_UNINI;
   list->sizeOfArray = 0;
}

enum data_structure_error arraylist_clone(const ArrayList * const list, ArrayList ** const new_list)
{
   if(list == NULL)
      goto FAIL0;

   /*create an ArrayList struct*/
   *new_list = malloc(sizeof(**new_list));
   if(*new_list == NULL)
      goto FAIL1;

   /*copy data to new struct*/
   memcpy(*new_list, list, sizeof(**new_list));

   /*allocate space for new array in the new struct*/
   (*new_list)->array = malloc(list->sizeOfElement * list->lengthOfArray);
   if((*new_list)->array == NULL)
      goto FAIL1;

   /*copy the data from the old array to new array*/
   memcpy((*new_list)->array, list->array, list->sizeOfElement * list->sizeOfArray);

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   free(*new_list);
   return E_MEMORY_FAIL;
}

enum data_structure_error arraylist_size(const ArrayList * const list, uint32_t * const size)
{
   if(list == NULL)
      return E_VAR_UNINI;
   *size = list->sizeOfArray;
}

enum data_structure_error arraylist_isempty(const ArrayList * const list, bool * const empty)
{
   if(list == NULL)
      return E_VAR_UNINI;
   *empty =  list->sizeOfArray == 0 ? 1 : 0;
}

enum data_structure_error arraylist_set_compare(ArrayList * const list, int32_t (*compare)(const void *, const void *))
{
   if(list == NULL)
      return E_VAR_UNINI;
   list->defaultCompare = compare;
}

enum data_structure_error arraylist_set_print(ArrayList * const list, void (*print)(const void *))
{
   if(list == NULL)
      return E_VAR_UNINI;
   list->defaultPrint = print;
}

enum data_structure_error arraylist_print_index(const ArrayList * const list, void(*print)(const void *), const uint32_t index)
{
   uint8_t *array;
   void *temp_storage;

   /*error checking*/
   if(list == NULL)
      goto FAIL0;
   else if(print == NULL && list->defaultPrint == NULL)
      goto FAIL1;

   temp_storage = malloc(list->sizeOfElement);
   if(temp_storage == NULL)
      goto FAIL2;

   /*Get the data from the array*/
   array = list->array;
   memcpy(temp_storage, array + index * list->sizeOfElement, list->sizeOfElement);

   /*call the correct print function*/
   if(print == NULL)
      list->defaultPrint(temp_storage);
   else
      print(temp_storage);


   free(temp_storage);
   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_PRINT_UNINI;
   FAIL2:
   return E_MEMORY_FAIL;
}

enum data_structure_error arraylist_print(const ArrayList * const list, void(*print)(const void *))
{
   uint32_t i;
   uint8_t *array;
   void *temp_storage;

   /*error checking*/
   if(list == NULL)
      goto FAIL0;
   else if(print == NULL && list->defaultPrint == NULL)
      goto FAIL1;

   /*allocate temp storage*/
   temp_storage = malloc(list->sizeOfElement);
   if(temp_storage == NULL)
      goto FAIL2;

   /*copying pointer of array*/
   array = list->array;

   /*loop through array and call the correct print function*/
   for(i = 0; i < list->sizeOfArray; i++)
   {
      memcpy(temp_storage, (array + i * list->sizeOfElement), list->sizeOfElement);
      if(print == NULL)
         list->defaultPrint(temp_storage);
      else
         print(temp_storage);
   }

   free(temp_storage);
   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_PRINT_UNINI;
   FAIL2:
   return E_MEMORY_FAIL;
}

static enum data_structure_error arraylist_resize(ArrayList * const list)
{
   void *new_array;

   /*try to allocate more memory for the array*/
   new_array = realloc(list->array, list->sizeOfElement * list->lengthOfArray * RESIZE_MULTIPLIER);
   if(new_array == NULL)
      goto FAIL0;

   /*update the struct*/
   list->array = new_array;
   list->lengthOfArray = list->lengthOfArray * RESIZE_MULTIPLIER;

   return E_SUCCESS;

   FAIL0:
   return E_MEMORY_FAIL;
}

enum data_structure_error arraylist_add_index(ArrayList * const list, const void * const ptr_to_data, const uint32_t index)
{
   uint8_t *array;
   uint8_t *array_two;
   enum data_structure_error error = E_SUCCESS;

   /*error handeling*/
   if(list == NULL || ptr_to_data == NULL)
      goto FAIL0;
   else if(index > list->sizeOfArray)
      goto FAIL1;
   else if(list->sizeOfArray >= list->lengthOfArray)
   {
      error = arraylist_resize(list);
      if(error != E_SUCCESS)
         goto FAIL2;
   }

   /*copying pointer of array*/
   array = array_two = list->array;

   /*the spot where the new value will be inserted*/
   array = array + list->sizeOfElement * index;

   /*the spot where all the elements will be shifted to*/
   array_two = array_two + list->sizeOfElement * (index + 1);

   /*shifting elements*/
   memmove(array_two, array, list->sizeOfElement * (list->sizeOfArray - index));

   /*inserting value*/
   memcpy(array, ptr_to_data, list->sizeOfElement);
   list->sizeOfArray++;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_OUT_OF_BOUNDS;
   FAIL2:
   return error;
}

enum data_structure_error arraylist_add_all_index(ArrayList * const list, const void * const ptr_to_array_data, const uint32_t length, const uint32_t index)
{
   uint8_t *array;
   uint8_t *array_two;
   enum data_structure_error error = E_SUCCESS;

   /*error checking*/
   if(list == NULL || ptr_to_array_data == NULL)
      goto FAIL0;
   else if(index > list->sizeOfArray)
      goto FAIL1;
   else if(list->sizeOfArray >= list->lengthOfArray)
   {
      error = arraylist_resize(list);
      if(error != E_SUCCESS)
         goto FAIL2;
   }

   /*making sure array has enough space*/
   while(length >= list->lengthOfArray - list->sizeOfArray)
   {
      error = arraylist_resize(list);
      if(error != E_SUCCESS)
         goto FAIL2;
   }


   /*copying pointer of array*/
   array = array_two = list->array;

   /*the spot where the new value/values will be inserted*/
   array = array + list->sizeOfElement * index;

   /*the spot where all the values will be shifted*/
   array_two = array_two + list->sizeOfElement * (index + length);

   /*shifting values*/
   memmove(array_two, array, list->sizeOfElement * (list->sizeOfArray - index));

   /*inserting new values into position*/
   memcpy(array, ptr_to_array_data, list->sizeOfElement * length);
   list->sizeOfArray = list->sizeOfArray + length;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_OUT_OF_BOUNDS;
   FAIL2:
   return error;
}

enum data_structure_error arraylist_add_all(ArrayList * const list, void *ptr_to_array_data, const uint32_t length)
{
   /*adding to end of the arraylist*/
   return arraylist_add_all_index(list, ptr_to_array_data, length, list->sizeOfArray);
}

enum data_structure_error arraylist_add(ArrayList * const list, const void * const ptr_to_data)
{
   /*adding to end of the arraylist*/
   return arraylist_add_index(list, ptr_to_data, list->sizeOfArray);
}

enum data_structure_error arraylist_get_index(const ArrayList * const list, void * const ptr_to_data, const uint32_t index)
{
   uint8_t *array;

   /*error checking*/
   if(list == NULL || ptr_to_data == NULL)
      goto FAIL0;
   else if(index >= list->sizeOfArray)
      goto FAIL1;

   /*copying array pointer*/
   array = list->array;

   /*position of the value to get*/
   array = array + list->sizeOfElement * index;

   /*copying data*/
   memcpy(ptr_to_data, array, list->sizeOfElement);

   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_OUT_OF_BOUNDS;
}

enum data_structure_error arraylist_contains(const ArrayList * const list, const void * const ptr_to_data, bool * const contains, int32_t (*compare)(const void *, const void *))
{
   uint32_t i;
   uint8_t *array;
   void *temp_storage;
   int32_t (*temp_compare)(const void *, const void *);


   /*error cheking*/
   if(list == NULL || ptr_to_data == NULL || contains == NULL)
      goto FAIL0;
   else if(compare == NULL && list->defaultCompare == NULL)
      goto FAIL1;

   /*allocating space*/
   temp_storage = malloc(list->sizeOfElement);
   if(temp_storage == NULL)
      goto FAIL2;

   /*if compare is not passed in use default compare*/
   if(compare == NULL)
      temp_compare = list->defaultCompare;
   else
      temp_compare = compare;


   array = list->array;
   /*set to false*/
   *contains = 0;
   for(i = 0; i < list->sizeOfArray; i++)
   {
      memcpy(temp_storage, array + list->sizeOfElement * i, list->sizeOfElement);
      if(temp_compare(temp_storage, ptr_to_data) == 0)
      {
         *contains = 1;
         break;
      }
   }

   free(temp_storage);
   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_COMPARE_UNINI;
   FAIL2:
   return E_MEMORY_FAIL;
}

enum data_structure_error arraylist_indexof(const ArrayList * const list, const void * const ptr_to_data, int32_t * const index, int32_t (*compare)(const void *, const void *))
{
   uint32_t i;
   uint8_t *array;
   void *temp_storage;
   int32_t (*temp_compare)(const void *, const void *);

   /*error checking*/
   if(list == NULL || ptr_to_data == NULL || index == NULL)
      goto FAIL0;
   else if(compare == NULL && list->defaultCompare == NULL)
      goto FAIL1;

   temp_storage = malloc(list->sizeOfElement);
   if(temp_storage == NULL)
      goto FAIL2;

   /*if compare is not passed in use default compare*/
   if(compare == NULL)
      temp_compare = list->defaultCompare;
   else
      temp_compare = compare;

   array = list->array;
   *index = -1;
   for(i = 0; i < list->sizeOfArray; i++)
   {
      memcpy(temp_storage, array + list->sizeOfElement * i, list->sizeOfElement);
      if(temp_compare(temp_storage, ptr_to_data) == 0)
      {
         *index = i;
         break;
      }
   }

   free(temp_storage);
   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_COMPARE_UNINI;
   FAIL2:
   return E_MEMORY_FAIL;
}

enum data_structure_error arraylist_lastindexof(const ArrayList * const list, const void * const ptr_to_data, int32_t * const index, int32_t (*compare)(const void *, const void *))
{
   int32_t i;
   uint8_t *array;
   void *temp_storage;
   int32_t (*temp_compare)(const void *, const void *);

   /*error checking*/
   if(list == NULL || ptr_to_data == NULL || index == NULL)
      goto FAIL0;
   else if(compare == NULL && list->defaultCompare == NULL)
      goto FAIL1;

   temp_storage = malloc(list->sizeOfElement);
   if(temp_storage == NULL)
      goto FAIL2;

   /*if compare is not passed in use default compare*/
   if(compare == NULL)
      temp_compare = list->defaultCompare;
   else
      temp_compare = compare;

   array = list->array;
   *index = -1;
   for(i = list->sizeOfArray - 1; i > -1; i--)
   {
      memcpy(temp_storage, array + list->sizeOfElement * i, list->sizeOfElement);
      if(temp_compare(temp_storage, ptr_to_data) == 0)
      {
         *index = i;
         break;
      }
   }

   free(temp_storage);
   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_COMPARE_UNINI;
   FAIL2:
   return E_MEMORY_FAIL;
}

enum data_structure_error arraylist_set_index(const ArrayList * const list, const void * const ptr_to_data, const uint32_t index)
{
   uint32_t i;
   uint8_t *array;

   /*error handeling*/
   if(list == NULL || ptr_to_data == NULL)
      goto FAIL0;
   else if(index >= list->sizeOfArray)
      goto FAIL1;

   /*get pointer to array*/
   array = list->array;

   /*set the data*/
   memcpy(array + list->sizeOfElement * index, ptr_to_data, list->sizeOfElement);

   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_OUT_OF_BOUNDS;
}

enum data_structure_error arraylist_remove_index(ArrayList * const list, void * const ptr_to_data, const uint32_t index)
{
   uint8_t *array;
   uint8_t *array_two;

   /*error handeling*/
   if(list == NULL)
      goto FAIL0;
   else if(index >= list->sizeOfArray)
      goto FAIL1;

   /*get pointer to array*/
   array = array_two = list->array;

   /*set pointer to data to remove*/
   array = array + list->sizeOfElement * index;

   /*set pointer to index + 1*/
   array_two = array_two + list->sizeOfElement * (index + 1);

   /*get the data if pointer is not NULL*/
   if(ptr_to_data != NULL)
      memcpy(ptr_to_data, array + list->sizeOfElement * index, list->sizeOfElement);

   /*shift the data*/
   memcpy(array, array_two, list->sizeOfElement * (list->sizeOfArray - (index + 1)));
   list->sizeOfArray = list->sizeOfArray - 1;

   return E_SUCCESS;

   FAIL0:
   return E_VAR_UNINI;
   FAIL1:
   return E_OUT_OF_BOUNDS;
}

void arraylist_free(ArrayList * const list)
{
   free(list->array);
   free(list);
}





/*user code*/
void print_my_data(const void *data)
{
   int32_t *temp;
   temp = (int32_t *)data;
   printf("%d\n", *temp);
}

int compare_my_data(const void *ptr1, const void *ptr2)
{
   const int *data1;
   const int *data2;

   data1 = ptr1;
   data2 = ptr2;

   if(*data1 < *data2)
      return -1;
   else if(*data1 > *data2)
      return 1;

   return 0;
}

int main(void)
{
   ArrayList *list;
   arraylist_create(&list, sizeof(int), &compare_my_data, &print_my_data);

   for(int i = 0; i < 10; i++)
   {
      arraylist_add(list, &i);
   }

   arraylist_print(list, NULL);
   arraylist_free(list);
   return 0;
}
```
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

How I Review Code

I have performed Code Reviews for over 30 years both on the Code Review Community and professionally. I have also had to maintain code written by others and this generally requires a thorough code inspection before starting.

The criteria I use for code reviews is:

  1. Does the code compile without errors and warnings?
  2. Does the code work as expected?
  3. Is the code easy to maintain, especially by others?
  4. Is the code portable (will it compile and run on multiple systems, Window, Linux and Unix)?
  5. Does the code follow best practices? Usually this goes hand in hand with number 3.
  6. How complex is the code? (Really also part of number 3)
  7. Is the code extendable?
  8. Is the code reusable? (Can this code be used by multiple different programs without modification?)

General Observations

The code is well organized and doesn't need nor have function prototypes.

The code does consider some optimizations such as const parameters. (Good!)

Memory allocation is checked. (Good!)

The code compiles and runs on Windows 10 in Visual Studio 2019. Based on what I see the code should be portable to Linux and Unix as well. I do see 5 warning messages listed below.

The type names, variable names and function names all seem pretty clear which will make this code easier to maintain by others. Personally I would like to see the Global Variable errordesc renamed to error_description but it isn't totally necessary.

The code could definitely be reusable if some editing and file reoganization were performed. Right now some of the global variables could possibly conflict with other modules at link time. If the variables only need to be used within the file that implements the arraylist then declare them static within arraylist.c. If the global variables need to be global create a global (extern) declaration in a header file (probably called arraylist.h) and define the variable in arraylist.c. Put all global entry points (functions) in the header file as function prototypes.

Suggested Corrections

Abuse of Goto Statements

The good news is that the code only uses goto statements for error checking. The bad news is that there are some many goto statements in the code that aren't necessary. Some people feel that the use of goto statements outside of assembly code is evil, I'm not quite that bad, but there really isn't any reason to use goto statements in this code, and the goto statements are adding extra lines of code to the functions they are in:

Current implementation:

enum data_structure_error arraylist_create(ArrayList** const user_list, const size_t size_of_element, int32_t(*compare)(const void*, const void*), void (*print)(const void*))
{
    /*making 1 struct of type ArrayList*/
    ArrayList* list;

    if (user_list == NULL)
        goto FAIL0;

    list = malloc(sizeof(*list));
    if (list == NULL)
        goto FAIL1;

    /*making an array of void pointers*/
    list->array = malloc(size_of_element * ARRAY_LIST_DEFAULT_SIZE);
    if (list->array == NULL)
        goto FAIL1;

    /*setting all default values of ArrayList struct*/
    list->sizeOfElement = size_of_element;
    list->sizeOfArray = 0;
    list->lengthOfArray = ARRAY_LIST_DEFAULT_SIZE;
    list->defaultCompare = compare;
    list->defaultPrint = print;

    *user_list = list;
    return E_SUCCESS;

FAIL0:
    return E_VAR_UNINI;
FAIL1:
    free(list);
    return E_MEMORY_FAIL;
}

Without goto statements:

Data_structure_error alt_arraylist_create(ArrayList** const user_list, const size_t size_of_element, int32_t(*compare)(const void*, const void*), void (*print)(const void*))
{
    /*making 1 struct of type ArrayList*/
    ArrayList* list;

    if (user_list == NULL)
        return E_VAR_UNINI;

    list = malloc(sizeof(*list));
    if (list == NULL)
        return E_MEMORY_FAIL;        // No need to free list here.

    /*making an array of void pointers*/
    list->array = malloc(size_of_element * ARRAY_LIST_DEFAULT_SIZE);
    if (list->array == NULL)
    {
        free(list);
        return E_MEMORY_FAIL;
    }

    /*setting all default values of ArrayList struct*/
    list->sizeOfElement = size_of_element;
    list->sizeOfArray = 0;
    list->lengthOfArray = ARRAY_LIST_DEFAULT_SIZE;
    list->defaultCompare = compare;
    list->defaultPrint = print;

    *user_list = list;
    return E_SUCCESS;
}

Remove all Compiler Warning Messages

I use a strict compilation of C and there are some warning messages that should be corrected:

1>CArrayList.c(100): warning C4715: 'arraylist_clear': not all control paths return a value
1>CArrayList.c(142): warning C4715: 'arraylist_isempty': not all control paths return a value
1>CArrayList.c(135): warning C4715: 'arraylist_size': not all control paths return a value
1>CArrayList.c(149): warning C4715: 'arraylist_set_compare': not all control paths return a value
1>CArrayList.c(156): warning C4715: 'arraylist_set_print': not all control paths return a value

A function that returns a value should always return a value. The fact that is happens in 5 functions indicate that this is not a typo, but a systemic error.

A good way to find all possible issues is to compile using the -wall flag.

Use More Programmer Defined Types

The size of the code could be reduced if there were more typedefs, the code already contains a typedef for struct alt_arraylist_t but the follow examples could also ease coding:

typedef enum data_structure_error {
    E_SUCCESS = 0,
    E_FAIL = -1,
    E_MEMORY_FAIL = -2,
    E_VAR_UNINI = -3,
    E_VAR_TO_SMALL = -4,
    E_OUT_OF_BOUNDS = -5,
    E_PRINT_UNINI = -6,
    E_COMPARE_UNINI = -7
} Data_structure_error;

typedef struct data_structure_errordesc {
    const int32_t  code;
    const char* message;
} Data_structure_errordesc;

static const Data_structure_errordesc errordesc[] = {
   {E_SUCCESS, "No error" },
   {E_FAIL, "PANIC!!!!" },
   {E_MEMORY_FAIL, "Could not allocate memory"},
   {E_VAR_UNINI, "The variable passed in is uninitialized"},
   {E_VAR_TO_SMALL, "The variable passed in is too small to hold data"},
   {E_OUT_OF_BOUNDS, "Value passed in was out of bounds"},
   {E_PRINT_UNINI, "The default and the passed in print is NULL"},
   {E_COMPARE_UNINI, "The default and the passed in compare in NULL"}
};

Altered Function Declarations:

void alt_arraylist_error(Data_structure_error error);
Data_structure_error arraylist_create(ArrayList** const user_list, const size_t size_of_element, int32_t(*compare)(const void*, const void*), void (*print)(const void*));
Data_structure_error arraylist_clear(ArrayList* const list);

Use Table Lookup Rather Than Field Matching For Error Descriptions

Since the error enums are all negative numbers field matching is required in

void alt_arraylist_error(Data_structure_error error)
{
    uint32_t i;
    for (i = 0; i < sizeof(errordesc) / sizeof(errordesc[0]); i++)
    {
        if (error == errordesc[i].code)
        {
            printf("%s", errordesc[i].message);
            return;
        }
    }
}

However the code could be simplified if the enums were all positive numbers, and it would be faster as well

typedef enum data_structure_error {
    E_SUCCESS,      // Defaults to zero
    E_FAIL,         // Defaults to one
    E_MEMORY_FAIL,
    E_VAR_UNINI,
    E_VAR_TO_SMALL,
    E_OUT_OF_BOUNDS,
    E_PRINT_UNINI,
    E_COMPARE_UNINI
} Data_structure_error;

static char* errordesc[] = {
   {"No error" },                                           // E_SUCCESS
   {"PANIC!!!!" },                                          // E_FAIL
   {"Could not allocate memory"},                           // E_MEMORY_FAIL
   {"The variable passed in is uninitialized"},             // E_VAR_UNINI
   {"The variable passed in is too small to hold data"},    // E_VAR_TO_SMALL
   {"Value passed in was out of bounds"},                   // E_OUT_OF_BOUNDS
   {"The default and the passed in print is NULL"},         // E_PRINT_UNINI
   {"The default and the passed in compare in NULL"}        // E_COMPARE_UNINI
};

void arraylist_error(Data_structure_error error)
{
    printf("%s", errordesc[error]);
    return;
}

This would also make the code easier to add error values and error descriptions (ease of maintenance). It also removes the necessity of the struct

struct data_structure_errordesc {
    const int32_t  code;
    const char* message;
};
\$\endgroup\$
1
  • \$\begingroup\$ Pedantically speaking, the code does have function prototypes - the definitions are all also prototypes. \$\endgroup\$ Commented Jul 1, 2021 at 5:00
2
\$\begingroup\$

You have a great review by pacmaninbw, but I'll just pick up on one aspect and take it a little further.

There's some quite involved machinery for printing error messages:

struct data_structure_errordesc{
    const int32_t  code;
    const char *message;
};

const struct data_structure_errordesc errordesc[] = {
   {E_SUCCESS, "No error" },
   {E_FAIL, "PANIC!!!!" },
   {E_MEMORY_FAIL, "Could not allocate memory"},
   {E_VAR_UNINI, "The variable passed in is uninitialized"},
   {E_VAR_TO_SMALL, "The variable passed in is too small to hold data"},
   {E_OUT_OF_BOUNDS, "Value passed in was out of bounds"},
   {E_PRINT_UNINI, "The default and the passed in print is NULL"},
   {E_COMPARE_UNINI, "The default and the passed in compare in NULL"}
};

void arraylist_error(enum data_structure_error error)
{
   uint32_t i;
   for(i = 0; i < sizeof(errordesc) / sizeof(errordesc[0]); i++)
   {
      if(error == errordesc[i].code)
      {
         printf("%s", errordesc[i].message);
         return;
      }
   }
}

Instead of searching in a list, it's much simpler to use switch/case:

const char *arraylist_error_string(enum data_structure_error error)
{
    switch (error) {
        case E_SUCCESS: return "No error";
        case E_FAIL: return "PANIC!!!!";
        case E_MEMORY_FAIL: return "Could not allocate memory";
        case E_VAR_UNINI: return "The variable passed in is uninitialized";
        case E_VAR_TO_SMALL: return "The variable passed in is too small to hold data";
        case E_OUT_OF_BOUNDS: return "Value passed in was out of bounds";
        case E_PRINT_UNINI: return "The default and the passed in print is NULL";
        case E_COMPARE_UNINI: return "The default and the passed in compare in NULL";
    }
    /* not a valid enum value */
    return "Erroneous error!";
}

The advantage of this is that if we add another value to the enum and forget to update the strings, we can get a compiler warning (I promote these to errors), rather than just producing no message. Important - we must fall off the end of the switch in that case, because a default label will prevent the compiler warning.

Another benefit is that this code is easier to internationalise, by wrapping the literal strings with calls to gettext() or equivalent. It's harder to do that with a compile-time constant array.

(I would probably also correct the spelling of E_VAR_TOO_SMALL before letting any users get their hands on this, too! Good idea also to fix the typo in compare in NULL, too, but that's less urgent.)

I made the function return a string rather than printing to stdout because the user may require something different (print to stderr with a newline, display with a GUI, or send to syslog, for example).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.