My pointers and struct code is displaying garbage values

Question

Currently, I am trying to create a program that takes inventory of a stores products. I have been able to create a function that allows the user to input new items in a struct array but when I attempt to print the values I get garbage values. (Please ignore the switch statements as the code is work in progress).

#include <stdio.h>
#include <stdlib.h>
#define MAX_INVENTORY_SIZE 100

typedef struct {
    char item_Number[3];
    char item_Name[20];
    float item_Profit;
    float latest_Price;
    unsigned int stock;
    unsigned int total_Sold;
    struct InventoryItemType *next;
}InventoryItemType;
void MainMenu();
void displayInventory(InventoryItemType *(*));
void addItem(InventoryItemType, int i);

int main()
{
    int i=1;
    char selection;
    int count=1;
    InventoryItemType *inventoryItems[MAX_INVENTORY_SIZE] ;

    inventoryItems[0]=NULL;
    while(1)
    {
        MainMenu();
        scanf(" %c", &selection);
        switch(selection) 
        {
        case 'A' :
            displayInventory(inventoryItems);
            break;
        case 'B' :
        case 'C' :
            inventoryItems[count]= (InventoryItemType*)malloc(sizeof(InventoryItemType));
            addItem(*inventoryItems[count], count);
            continue;
        case 'D' :
        case 'E' :
        case 'F' :
        case 'G' :
        case 'H' :
        default :
            printf("Invalid\n" );
        }
        printf("Bottom of code\n");
        system("pause");
    }
}
void MainMenu()
{
    printf("A. Display Inventory\n");
    printf("B. Display Sales\n");
    printf("C. Add Item\n");
    printf("D. Remove Item\n");
    printf("E. Enter Shipment\n");
    printf("F. Update Sales\n");
    printf("G. Sort\n");
    printf("H. Exit\n");
    printf("Make a selection\n");
}
void displayInventory(InventoryItemType *display)
{
    int i;
    for(i=0; i<MAX_INVENTORY_SIZE; i++)
    {
        printf("Name:%s\n", display[i].item_Name);
        printf("Stock:%d\n", display[i].stock);
        printf("Price:%.2f\n", display[i].latest_Price);
        printf("Total Value:%.2f\n", (display[i].stock)*(display[i].latest_Price));
        printf("\n");
    }
}
void addItem(InventoryItemType display)
{

    printf("\nEnter details of item \n\n");
    printf("Enter Item Name: \n");
    scanf("%s", display.item_Name);
    printf("\nEnter Item no: \n");
    scanf("%s", display.item_Number);

    printf("\nEnter Stock: \n");
    scanf("%d", &display.stock);

    printf("\nPurchase Price: \n");
    scanf("%f", &display.latest_Price);
}

Update

I attempted to apply all answers (with a series of trial and error) and came up with this code. I currently have the code properly taking in a new item and displaying that item, but it breaks down after adding more items(if I continue through the errors I get garbage display values). I am pretty certain it has to do with the way in which I am allocating memory via malloc. My goal is to use malloc to create the space for the item before initializing values.

Edited code:

#include <stdio.h>
#include <stdlib.h>
#define MAX_INVENTORY_SIZE 100

typedef struct {
    char item_Number[4];
    char item_Name[20];
    float item_Profit;
    float latest_Price;
    float selling_Price;
    unsigned int stock;
    unsigned int total_Sold;
}InventoryItemType;
void MainMenu();
void displayInventory(InventoryItemType *, int);
void displaySales(InventoryItemType *, int);
InventoryItemType *addItem(InventoryItemType *, int);

int main()
{
    int i=0, item_count=0;
    char selection;
    InventoryItemType *inventoryItems[MAX_INVENTORY_SIZE];
    while(1)
    {
        MainMenu();
        scanf(" %c", &selection);
        switch(selection) 
        {
        case 'A' :
            displayInventory(*inventoryItems, item_count);
            break;
        case 'B' :
            displaySales(*inventoryItems, item_count);
            break;
        case 'C' :
            inventoryItems[item_count]=(InventoryItemType*)malloc(sizeof(InventoryItemType));
            inventoryItems[item_count]=addItem(inventoryItems[item_count],item_count);
            item_count++;
            continue;
        case 'D' :
        case 'E' :
        case 'F' :
        case 'G' :
        case 'H' :
        default :
            printf("Invalid Entry\n" );
            system("pause");
        }
        system("cls");
    }
}
void MainMenu()
{
    printf("A. Display Inventory\n");
    printf("B. Display Sales\n");
    printf("C. Add Item\n");
    printf("D. Remove Item\n");
    printf("E. Enter Shipment\n");
    printf("F. Update Sales\n");
    printf("G. Sort\n");
    printf("H. Exit\n");
    printf("Make a selection\n");
}
void displayInventory(InventoryItemType *display, int key)
{
    if(display[0].item_Name == NULL)
    {
        printf("No stock");
        system("pause");
    }
    else
    {
        int i;
        for(i=0; i<key; i++)
        {
            printf("Item No.:%s\n", display[i].item_Number);
            printf("Item Name:%s\n", display[i].item_Name);
            printf("Item Stock:%d\n", display[i].stock);
            printf("Item Purchased Price:%.2f\n", display[i].latest_Price);
            printf("Total Value of Items:%.2f\n", (display[i].stock)*(display[i].latest_Price));
            printf("\n");
            system("pause");
        }
    }
}
void displaySales(InventoryItemType *display, int key)
{
    int i;
    float total_profit=0;
    for(i=0; i<key; i++)
    {
        printf("Item No.:%s\n", display[i].item_Number);
        printf("Item Name:%s\n", display[i].item_Name);
        printf("Number of Item Sold:%d\n", display[i].total_Sold);
        printf("Item Selling Price:%.2f\n", display[i].selling_Price);
        printf("Total Profit from Item:%.2f\n", (display[i].selling_Price-display[i].latest_Price)*display[i].total_Sold);
        total_profit=total_profit+((display[i].selling_Price-display[i].latest_Price)*display[i].selling_Price);
        if(i==key)
        printf("Total Over-all Profit:%.2f", total_profit);
        system("pause");
    }
}
InventoryItemType *addItem(InventoryItemType *change, int key)
{
    InventoryItemType *current= (InventoryItemType*)malloc(sizeof(InventoryItemType));
    printf("\nEnter details of item \n\n");
    printf("Enter Item no: \n");
    scanf("%s", current[key].item_Number);
    printf("Enter Item Name: \n");
    scanf("%s", current[key].item_Name);
    printf("Enter Stock: \n");
    scanf("%d", &current[key].stock);
    printf("Enter Purchase Price: \n");
    scanf("%f", &current[key].latest_Price);
    current[key].selling_Price=(current[key].latest_Price)*1.5;
    current[key].total_Sold=0;
    change=current;
    system("cls");
    return change;
}

This shouldn't even compile. InventoryItemType *inventoryItems[MAX_INVENTORY_SIZE] ; you have an array of pointers, but call displayInventory(inventoryItems); when displayInventory takes a InventoryItemType *. — Mooing Duck
– Mooing Duck, Commented Mar 28, 2018 at 21:05
you have an array of InventoryItemType pointers.. I suspect you want an array of InventoryItemType objects. — yano
– yano, Commented Mar 28, 2018 at 21:05
@yano Looks like OP is mallocing the elements of inventoryItems, so I don't think that is the case. I'm more curious about what's happening in addItem. — Christian Gibbons
– Christian Gibbons, Commented Mar 28, 2018 at 21:11
@ChristianGibbons Oh you're right... Still with a (relatively small) array I'd prefer to use objects in automatic storage here rather than mallocing each one. — yano
– yano, Commented Mar 28, 2018 at 21:17
@ChristianGibbons addItem is supposed to add a new inventory item which should include the name of the item, item number(arbitrary but not repeatable), quantity (stock), and purchase price. Please have mercy I am very new to struct, arrays, and pointers. — Trial'n'Error
– Trial'n'Error, Commented Mar 28, 2018 at 21:18

Craig Estey · Accepted Answer · 2018-03-28 21:33:19Z

There are a number of bugs/conflicts. Too many to comment on individually. I've annotated and corrected the code, showing bugs and before/after. There is still more work to do, but this should get you closer.

#include <stdio.h>
#include <stdlib.h>
#define MAX_INVENTORY_SIZE 100

typedef struct {
    char item_Number[3];
    char item_Name[20];
    float item_Profit;
    float latest_Price;
    unsigned int stock;
    unsigned int total_Sold;
    struct InventoryItemType *next;
} InventoryItemType;

void MainMenu();

#if 0
void displayInventory(InventoryItemType *(*));
#else
void displayInventory(InventoryItemType *,int);
#endif

#if 0
void addItem(InventoryItemType, int i);
#else
void addItem(InventoryItemType *);
#endif

int main()
{
    int i=1;
    char selection;

    // NOTE/BUG: starting at 1 will leave item 0 undefined
#if 0
    int count=1;
#else
    int count=0;
#endif

    // better to do this with a stack array than pointers to malloc'ed items
#if 0
    InventoryItemType *inventoryItems[MAX_INVENTORY_SIZE] ;
#else
    InventoryItemType inventoryItems[MAX_INVENTORY_SIZE] ;
#endif

#if 0
    // NOTE/BUG: this will segfault because inventoryItems is undefined here
    inventoryItems[0]=NULL;
#endif

    while (1) {
        MainMenu();
        scanf(" %c", &selection);
        switch(selection) {
        case 'A' :
#if 0
            displayInventory(inventoryItems);
#else
            displayInventory(inventoryItems,count);
#endif
            break;
        case 'B' :
        case 'C' :
#if 0
            inventoryItems[count]= (InventoryItemType*)malloc(sizeof(InventoryItemType));
            addItem(*inventoryItems[count], count);
#else
            addItem(&inventoryItems[count]);
            count += 1;
#endif
            continue;
        case 'D' :
        case 'E' :
        case 'F' :
        case 'G' :
        case 'H' :
        default :
            printf("Invalid\n" );
        }
        printf("Bottom of code\n");
        system("pause");
    }
}

void MainMenu()
{
    printf("A. Display Inventory\n");
    printf("B. Display Sales\n");
    printf("C. Add Item\n");
    printf("D. Remove Item\n");
    printf("E. Enter Shipment\n");
    printf("F. Update Sales\n");
    printf("G. Sort\n");
    printf("H. Exit\n");
    printf("Make a selection\n");
}

#if 0
void displayInventory(InventoryItemType display)
{
    int i;
    for(i=0; i<MAX_INVENTORY_SIZE; i++)
    {
        printf("Name:%s\n", display[i].item_Name);
        printf("Stock:%d\n", display[i].stock);
        printf("Price:%.2f\n", display[i].latest_Price);
        printf("Total Value:%.2f\n", (display[i].stock)*(display[i].latest_Price));
        printf("\n");
    }
}

void addItem(InventoryItemType display)
{

    // NOTE/BUG: because this is passed by _value_ nothing will be retained
    // after function return

    printf("\nEnter details of item \n\n");
    printf("Enter Item Name: \n");
    scanf("%s", display.item_Name);
    printf("\nEnter Item no: \n");
    scanf("%s", display.item_Number);

    printf("\nEnter Stock: \n");
    scanf("%d", &display.stock);

    printf("\nPurchase Price: \n");
    scanf("%f", &display.latest_Price);
}
#else
void displayInventory(InventoryItemType *item,int count)
{
    int i;
    for (i=0; i<count; i++, item++) {
        printf("Name:%s\n", item->item_Name);
        printf("Stock:%d\n", item->stock);
        printf("Price:%.2f\n", item->latest_Price);
        printf("Total Value:%.2f\n", (item->stock)*(item->latest_Price));
        printf("\n");
    }
}

void addItem(InventoryItemType *item)
{

    printf("\nEnter details of item \n\n");

    printf("Enter Item Name: \n");
    scanf("%s", item->item_Name);

    printf("\nEnter Item no: \n");
    scanf("%s", item->item_Number);

    printf("\nEnter Stock: \n");
    scanf("%d", &item->stock);

    printf("\nPurchase Price: \n");
    scanf("%f", &item->latest_Price);
}
#endif

Pablo · Accepted Answer · 2018-03-29 00:32:55Z

2

There are many problems with your code.

InventoryItemType *inventoryItems[MAX_INVENTORY_SIZE] ;

This declares an array of dimension MAX_INVENTORY_SIZE of InventoryItemType pointers. These pointers are uninitialized, so you need to allocate memory for it. You do it here:

inventoryItems[count]= (InventoryItemType*)malloc(sizeof(InventoryItemType));

Don't cast malloc
The count variable is initialized to 1 and it is never changed. So every time you are allocating new memory for inventoryItems[1] and you are losing the pointer of the previous malloc call, thus it's leaking.

When you print your data structure, you do:

for(i=0; i<MAX_INVENTORY_SIZE; i++)
    {
        printf("Name:%s\n", display[i].item_Name);
        ...

but inventoryItems[i] is for all i != 1 uninitialized, this yield undefined behaviour.

What's the point of having an array of InventoryItemType pointers when the structure itself has a pointer next pointer for the next item in the inventory. It seems you are mixing concepts here, linked lists vs arrays. I'd suggest you pick one and stick to it.

Why did you

void addItem(InventoryItemType display);

declare addItem like this? display is only a copy of the orginal, so any change of the values of display will only affect display, not the original. So when you do

addItem(*inventoryItems[count], count);

you are passing a copy to addItem and you only change the copy, inventoryItems[count] remains unchanged. Also your function takes only one parameter, you are passing it two.

addItem should take a pointer, so that the changes are done through the pointer into the original. The function should look like this:

int addItem(InventoryItemType *display);
{

    if(display == NULL)
        return 0;

    printf("\nEnter details of item \n\n");
    printf("Enter Item Name: \n");
    if(scanf("%19s", display->item_Name) != 1)
        return 0;

    clear_stdin();

    printf("\nEnter Item no: \n");
    if(scanf("%2s", display->item_Number) != 1)
        return 0;

    clear_stdin();

    printf("\nEnter Stock: \n");
    if(scanf("%d", &display->stock) != 1)
        return 0;

    printf("\nPurchase Price: \n");
    if(scanf("%f", &display->latest_Price) != 1)
        return 0;


    return 1;
}

The function should return 1 on success, 0 otherwise. The caller of addItem will know whether addItem failed or not. If you declare the function as void, then you will never know if the function failes or not.

And clear_stdin could look like this:

 void clear_stdin(void)
 {
     int c;
     while((c = getchar()) != '\n' && c != EOF);
 }

edit

As you've made an update and you've decided to use the array, then I can give you a few more pointers.

You are declaring to much memory. Either you allocate memory before addItem and pass the newly allocated memory to addItem, or you allocate the memory in addItem itself and return it. Your addItem already does that, so the first line

inventoryItems[item_count]=(InventoryItemType*)malloc(sizeof(InventoryItemType));

is not necessary, because addItem does that for you.

I'd change addItem to look like this:

InventoryItemType *addItem(void)
{
    InventoryItemType *current = calloc(1, sizeof *current);
    if(current == NULL)
        return NULL;

    printf("\nEnter details of item \n\n");
    printf("Enter Item no: \n");
    scanf("%2s", current->item_Number);

    clear_stdin();

    printf("Enter Item Name: \n");
    scanf("%20s", current->item_Name);

    clear_stdin();

    printf("Enter Stock: \n");
    scanf("%d", &current->stock);
    printf("Enter Purchase Price: \n");
    scanf("%f", &current->latest_Price);
    current->selling_Price=(current->latest_Price)*1.5;
    current->total_Sold=0;
    system("cls");

    return current;
}

Because addItem allocates the memory for the new object, you don't need to pass the array nor the index (or key) where you want to store it. Then in the main you can do this:

case 'C':
    // checking that you don't step outside of bounds
    if(item_count == MAX_INVENTORY_SIZE - 1)
    {
        fprintf(stderr, "Array is full\n");
        break;
    }

    inventoryItems[item_count] = addItem();
    if(inventoryItems[item_count] == NULL)
    {
        fprintf(stderr, "Not enough memory\n");
        break;
    }

    item_count++;
    continue;

case 'D':
    ...

edited Mar 29, 2018 at 0:32

answered Mar 28, 2018 at 21:40

Pablo

13.7k5 gold badges45 silver badges66 bronze badges

8 Comments

Trial'n'Error Over a year ago

How would I work around not casting Malloc? My goal is to create the memory required for a new item before initializing its values.

Pablo Over a year ago

@Trial'n'Error How would I work around not casting Malloc? by not casting it. My goal is to create the memory required for a new item before initializing its values I know that, but like I said in the answer, you are mixing concepts here. Do you want a linked list or an array. The correct way depends on what approach you want to take.

Trial'n'Error Over a year ago

Array, i have taken out the linked list.

Pablo Over a year ago

@Trial'n'Error are you compiling you C code with a C++ compiler? This might explain the error?

Pablo Over a year ago

@Trial'n'Error as far as I know, VS C compiler is a C++ compiler in disguise and that's why you need the cast in VS. The error you see is because you are passing to displayInventory the incorrect type, it should be: displayInventory(inventoryItems, item_count); and if(display[0].item_Name == NULL) will never be true because item_Name is not a pointer, is a char array. You have to get the types right, read the warnings of the compiler.

|

Collectives™ on Stack Overflow

My pointers and struct code is displaying garbage values

Update

2 Answers 2

Comments

8 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

Update

2 Answers 2

Comments

8 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related