C - iterating numberOfProducts variable does not work

I'm new to C and am creating an online store which manages a bunch of products in its inventory. I'm having trouble printing all my products out. When the user enters '1' which should iterate through all the elements in productCollection and print out each Product object, it prints nothing.


I think it's because the numberOfProducts is always 0 even though I iterate it in addProduct(). Despite where I place the iterator, when I print the numberOfProducts, it always prints 0. I was wondering why?




main.c

Code:

---

    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>


    #include "defs.h"


    void printInventory(ProductCollectionType*);
    int addProduct(ProductCollectionType*, int, char*, int, float);
   
    int main(){


        InventoryType store;
        store.storeName = "Walmart";
       
        ProductCollectionType p;
        p.numberOfProducts = 0;
       
        addProduct(&p, 1001, "Grape juice", 20, 12.5);
        addProduct(&p, 1002, "Apple juice", 30, 4.3);
       
       
        int choice;
        printf("(1) Print inventory \n");


        printf("\n");
        printf("Enter choice: ");
        scanf("%d", &choice);
     
        printf("%d \n", p.numberOfProducts);
     
        if(choice == 1){
            printInventory(&p);
        }
       
        return 0;




    }


    void printInventory(ProductCollectionType* productCollection){
        int i;
        for(int i = 0; i < productCollection->numberOfProducts; ++i){
            printf("Walmart Inventory");
            printf("Product #%d, %s, %d units, $ %.2f", productCollection->products[i]->id, productCollection->products[i]->name, productCollection->products[i]->numberOfUnits, productCollection->products[i]->price);
        }
    }


    int addProduct(ProductCollectionType* productCollection, int i, char* n, int u, float p){
        for(i = 0; i < productCollection->numberOfProducts; ++i){
            productCollection->products[i]->id = i;
            productCollection->products[i]->name = n;
            productCollection->products[i]->numberOfUnits = u;
            productCollection->products[i]->price = p;
           
            productCollection->numberOfProducts++;
        }
    }

---

defs.h

Code:

---

    #define MAX_NAME 40
    #define MAX_UNITS 15
    #define MAX_PROD 15
    typedef struct{
        int id;
        char *name;
        int numberOfUnits;
        float price;
    }ProductType;


    typedef struct {
        ProductType* products[MAX_UNITS];
        int numberOfProducts;
        int nextId;
    }ProductCollectionType;


    typedef struct{
        char *storeName;
        ProductCollectionType* productCollection[MAX_PROD];
    }InventoryType;

---

Maybe "ProductType* products[MAX_UNITS];" should be " ProductType* products[MAX_UNITS];", so rather than having an array of pointers to ProductType, you have an array of the ProductType structure.

Also the for loop in addProduct() seems wrong. Do you really want to set all of them to be the same product?

For addProduct() my attempt was to initialize the product's id, price, etc. but for every different product that is added. I thought increasing the index meant the id/price/units and such would only be initialized for that specific product/index in the productCollection array cause every product would have a different index?

I would expect to see (with excessive comments):

Code:

---

int addProduct(ProductCollectionType* productCollection, int i, char* n, int u, float p){
    // Get the next index number
    int i = productCollection->numberOfProducts;

    // Check we haven't used all the slots
    if(i == MAX_UNITS)
      return 0;  // Failure

    // We have one more product so update the numberOfProducts
    productCollection->numberOfProducts++;

    // Set all the product values
    productCollection->products[i].id = i;
    productCollection->products[i].name = n;
    productCollection->products[i].numberOfUnits = u;
    productCollection->products[i].price = p;
    return 1;  // Success
}

---

If we look at addProduct, it looks like it doesn't really add a product to the production collection. Rather, it iterates over the existing product collection and sets every existing product to the given values... and then it increments the number of products, which means that the loop just keeps going until you get an integer overflow for productCollection->numberOfProducts. However, because productCollection->numberOfProducts is initialised to 0, what happens in practice is that the loop never runs, so addProduct has no net effect. That explains why you observe no output: there's no output when printing the products because there are no products to print.

A few other thoughts:

• Your header file should have an inclusion guard.
• Rather than InventoryType, perhaps it should be StoreType, or just Store.
• Rather than ProductCollectionType, perhaps it should be InventoryType, or just Inventory (i.e., a product inventory is a collection of products).
• Are you sure you want products to be an array of ProductType pointers rather than an array of ProductType objects? There are good reasons to opt either way, but if you're new to C I would suggest sticking to an array of ProductType objects as you won't have to worry about allocating storage for the products (and deallocating when done).
• Are you sure you want the name member of ProductType to be a char* rather than a char[MAX_NAME] or char[MAX_NAME+1]? Having it as a char* right now is convenient because you're assigning string literals, but it is also error prone: attempting to modify string literals results in undefined behaviour, so the char* should more properly be a const char*.
• It is good for function parameters to have descriptive names so that it is easier to understand the function. On the same note, you should not remove the parameter names from the forward declaration of the function.

Hi so I basically made the following changes to my code but rather than a period I used an arrow for 'productCollection->products[i]->id = i' because the ProductType* products[MAX_UNITS] and ProductCollectionType* productCollection[MAX_PROD] both have to be an array of structure pointers. The issue is the 4 lines where I try to initialize the id, name, units, and price result in a segmentation fault and I'm not sure why. Once I comment these lines out, my program compiles but I'm not quite sure how I can initialize my variables without a seg fault.

Oh, I just realised that you have:

Code:

---

ProductCollectionType* productCollection[MAX_PROD];

---

This looks strange: I would expect MAX_PROD to refer to the maximum number of products, not the maximum number of product collections. With my given renaming suggestion, I might expect something like:

Code:

---

typedef struct{
    int id;
    char name[MAX_NAME + 1];
    int numberOfUnits;
    float price;
} Product;


typedef struct {
    Product products[MAX_PROD];
    int numberOfProducts;
    int nextId;
} Inventory;


typedef struct{
    char name[MAX_NAME + 1];
    Inventory inventory;
} Store;

---

So you'll still define MAX_UNITS, but it'll be used to validate the numberOfUnits value for each product. You can rename storeName to just name since it would be clear from context that it is the name of the store. I'm assuming all names have the same maximum length, but of course it could be the case that product names and store names might be expected to have differing maximum lengths.

EDIT:

Quote:

---

Originally Posted by eagerissac

Once I comment these lines out, my program compiles but I'm not quite sure how I can initialize my variables without a seg fault.

---

If you have an array of pointers, then you still need to allocate storage for the objects that the pointers will point to, e.g., by using malloc. This also means that when done, you should deallocate the storage (although if "when done" refers to the end of the program, with a typical modern OS you can just let the OS reclaim the memory, but it is good practice to do so yourself anyway).