Thread: C - iterating numberOfProducts variable does not work

  1. #1
    Registered User
    Join Date
    Oct 2020
    Posts
    11

    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;

  2. #2
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    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?

  3. #3
    Registered User
    Join Date
    Oct 2020
    Posts
    11
    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?

  4. #4
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    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
    }

  5. #5
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    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.
    Last edited by laserlight; 10-18-2020 at 07:50 PM.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #6
    Registered User
    Join Date
    Oct 2020
    Posts
    11
    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.

  7. #7
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    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).
    Last edited by laserlight; 10-18-2020 at 07:54 PM.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. While Loop not iterating
    By csmith03 in forum C Programming
    Replies: 6
    Last Post: 10-16-2017, 01:31 PM
  2. Help with iterating the menu !
    By thebenman in forum C Programming
    Replies: 4
    Last Post: 10-21-2014, 08:06 PM
  3. Iterating through a map that contains a vector
    By AngryStyro in forum C++ Programming
    Replies: 2
    Last Post: 06-08-2008, 05:01 AM
  4. Iterating through an array
    By MikeyIckey in forum C Programming
    Replies: 15
    Last Post: 11-10-2007, 10:26 PM
  5. Returning zero valued variable from function doesn't work
    By thetinman in forum C Programming
    Replies: 7
    Last Post: 10-19-2006, 05:53 PM

Tags for this Thread