I was trying to code something to practice the C langage. What I wanted to do (again only to practice, not for any use) was to create some kind of inventory system on which you could add an item in or take an item out.
I decided to implement it with a dynamic string array. Since I'm quite new to C, it was a hard time actualy understanding what was going on with the pointers and stuff, but I eventually got it.
I managed to code something. But I have an issue that I cannot seem to debug, I don't understand where it comes from, here's my code (probably really cursed)
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
int addItemToInventory(char ***Inv_content,int *Inv_length,char* Item)
{
int item_name_size=strlen(Item);
if (*Inv_length==0) //If The size of the inventory is 0, I have to create the array.
{
*Inv_content = malloc((1) * sizeof(char*));
if (*Inv_content) // malloc succeed
{
*Inv_content[0] = malloc((item_name_size+1) * sizeof(char));
if (*Inv_content[0]) // malloc succeed
{
strcpy(*Inv_content[0], Item);
*Inv_length+=1;
}
else // malloc failed
{
printf("Impossible to allocate memory for item '%s' in InvContent[0] \n", Item);
return(2);
}
}
else // malloc failed
{
printf("Impossible to allocate memory for Inv. (Create New Inv)\n");
return(1);
}
}
else //the size of the inventory is greater than zero, I have to expand the current array and add the item to the new slot
{
char **tmp_pnt;
tmp_pnt= realloc(*Inv_content, (*Inv_length+1) * sizeof(char*));
if (tmp_pnt)
{
*Inv_content=tmp_pnt;
*Inv_content[*Inv_length] = malloc((item_name_size+1) * sizeof(char));
if (*Inv_content[*Inv_length])
{
strcpy(*Inv_content[*Inv_length], Item);
*Inv_length+=1;
}
else // malloc failed
{
printf("unable to allocate memory for item '%s' in InvContent[%d]\n", Item, *Inv_length);
return(2);
}
}
else // realloc failed
{
printf("Impossible to allocate memory for Inv. (Add Item (realloc))\n");
return(1);
}
}
return(0);
}
So, I hope the code was clear enough, as you can see, this function takes 3 parameters :
a pointer to the inventory itself (In order to make changes directly to it)
a pointer to the size of the inventory, for the same reason
and the item name that we want to add
Now comes the issue, here's the main function I made to test my function:
int main()
{
//int run = 1;
char **Inv_content = NULL;
int Inv_length=0;
//int Item_index=-1;
char Item1[12]="Great Sword";
char Item2[8]="Big Bow";
addItemToInventory(&Inv_content, &Inv_length, Item1);
addItemToInventory(&Inv_content, &Inv_length, Item2);
printf("\n");
for (int i=0; i<=Inv_length-1;i++)
{
printf ("Item %d : %s \n", i, Inv_content[i]);
}
}
and here's the result:
When calling the function once, no problems, I figured that the first part of my function (Case when size = 0) works fine. When calling a second time, it outputs "(null)" as a result, like if there was nothing there in the array. and when calling a third time (or more), it's even worse, it crashes with "segmentation error".
Here's what I figured out, the second part works as intended, when printf("%s", *Inv_content[1]) inside the function even at the end of it, it outputs "Big bow", like intended, but as soon as it leaves the function, when printing inv_content[1] it outputs (null) like if it was wiped when the function exit.
Lastly, I don't understand why I've got a segmentation error when calling 3+ times, since it doesn't with 2 (which should be the same case)
Anyway, I hope you guys can help me understand a bit more what's happening so I could not make these mistakes in the future :) Thank you
For starters the function should be declared at least like
int addItemToInventory( char ***Inv_content, size_t *Inv_length, const char *Item );
Secondly there is no sense to split the body of the function into two parts depending on the condition
if (*Inv_length==0)
And the function contains bugs in statements like for example that
*Inv_content[*Inv_length] = malloc((item_name_size+1) * sizeof(char));
You need to write
( *Inv_content )[*Inv_length] = malloc((item_name_size+1) * sizeof(char));
The function can look the following way
int addItemToInventory( char ***Inv_content, size_t *Inv_length, const char *Item )
{
char **tmp_pnt = realloc( *Inv_content, ( *Inv_length +1 ) * sizeof( char* ) );
int result = tmp_pnt != NULL ? 0 : 1;
if ( result == 0 )
{
*Inv_content = tmp_pnt;
( *Inv_content )[*Inv_length] = malloc( strlen( item ) + 1 );
if ( ( *Inv_content )[*Inv_length] == NULL ) result = 2;
if ( result == 0 )
{
strcpy( ( *Inv_content )[*Inv_length], Item );
++*Inv_length;
}
}
return result;
}
Pay attention to that the function should not issue any message.
It is the caller of the function that will decide whether to output a message based on the return value of the function or not.
Also instead of using magic numbers like 0, 1, and 2 within the function you could introduce an enumeration as for example
enum { Success = 0, ReallocFailure = 1, MallocFailure = 2 };
and within the function write for example
int result = tmp_pnt != NULL ? Success : ReallocFailure;
if ( result == Success )
//... and so on
There is some serious problem with the function. If a character array for the string item was not allocated the array of pointers nevertheless was already reallocated. So the function behavior will be logically inconsistent.
It is better to rewrite the function the following way
enum { Success = 0, ReallocFailure = 1, MallocFailure = 2 };
int addItemToInventory( char ***Inv_content, size_t *Inv_length, const char *Item )
{
char *s = malloc( strlen( item ) + 1 );
int result = s != NULL ? Success : MallocFailure;
if ( result == Success )
{
char **tmp_pnt = realloc( *Inv_content, ( *Inv_length +1 ) * sizeof( char* ) );
if ( tmp_pnt == NULL ) result = ReallocFailure;
if ( result == Success )
{
strcpy( s, Item );
tmp_pnt[*Inv_length] = s;
*Inv_content = tmp_pnt;
++*Inv_length;
}
else
{
free( s );
}
}
return result;
}
That function looks more clear and has no logical inconsistence.