adding value to struct, adding goes wrong in C

Question

I'm new to programming so I'm trying to write a small program were I can show car information but also add cars to my "library" right now my out come for 1.Show cars looks like this:

  ID          BRAND         PICS 
  bbb188     BMW    1   2   3
  AAA-999     VOLVO    4   5   6
  CCC-999     CITROEN    1   2   3

but after I add a new car the PICS does not show. so if I would add AAA-111 VOLVO 1. this is the outcome:

 bbb188     BMW    1   2   3
 AAA-999     VOLVO    4   5   6
 CCC-999     CITROEN    1   2   3
 AAA-111     VOLVO    -398253632   3   3

I just get random numbers for pics and always 3 values. Could anyone help me with this, and please show me how to do it instead.

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

#define MAX 1000
#define IDSIZE 20
#define BRANDSIZE 50
#define PICSIZE 10

typedef struct{
  char id[IDSIZE+1];
  char brand[BRANDSIZE+1];
  int *pic;
} Car;


void printCar(Car *pCar,int imagecount)
{
  printf(" %s ",pCar->id);
  printf("    %s ",pCar->brand);
  for(int i=0; i<imagecount; i++){
    printf("   %d",pCar->pic[i]);
  }
  printf("\n");
}

Car initCar(char itsId[],char itsBrand[],int *itsPic, int imagecount)
{
  Car newCar;
  strcpy(newCar.id, itsId);
  strcpy(newCar.brand, itsBrand);
  newCar.pic = itsPic;

  return newCar;
}

void PrintList(Car aLista[],int imagecount, int carcount)
{
  for(int i = 0; i<imagecount; i++)
    printCar(&aLista[i],carcount);
}

void AddCar(Car aList[], int *pAt, Car theCar) 
{
  aList[(*pAt)++]=theCar;    
}

Car NewCar(Car minapatienter[], int patientCount)
{
  Car newCar;

  gets(newCar.id);
  printf("type in  ID \n"); 
  gets(newCar.id); 
  printf("type in brand\n");
  gets(newCar.brand);

  bool imageInputDone = false; 
  int imageCount=0;
  while(imageInputDone == false)
  {
    printf("type in image reference \n");
    int newImage; 
    scanf("%d",&newImage);

    newCar.pic = &newImage; 
    imageCount++;
    printf("vill du \n1.Add another image reference \n2.exit\n");
    int input;
    scanf("%d", &input);
    printf("input: %i\n",input);
    switch(input)
    {
        case 1: 
            printf("Adding one more image\n");
            break;
        case 2: 
            printf("Leaving loop\n");
            imageInputDone = true; 
            break;
        default:
            while (input<1 || input<2)
              ;
            printf("Input correct number\n");
            break;
    }

    return newCar; 
  }
}

int main(void)
{
  int carCount=0;
  int imagecount=0;
  Car myCar[MAX]; 
  int input;

  int test[3]={1,2,3};
  int test2[3]={4,5,6};

  myCar[0]= initCar("bbb188","BMW", test, 3);
  myCar[1] = initCar("AAA-999","VOLVO", test2, 3);
  myCar[2] = initCar("CCC-999", "CITROEN", test,3);

  carCount=3;
  imagecount=3;

  do {
    printf("1. Show cars \n2. Add car \n");
    scanf("%d",&input);
    switch(input)
    {
      case 1:
        printf("ID          BRAND         PICS \n");
        PrintList(myCar,carCount, imagecount);
        break; 
      case 2: 
        AddCar(myCar,&carCount,NewCar(myCar,carCount));
        printf("ID          BRAND         PICS \n");
        PrintList(myCar,carCount, imagecount);
    }    //break; 
  } while (input < '1'|| input < '2');

  return 0;
}

Welcome to SO. For better readability and attracting more users you should improve the indentation of your code. — Gerhardh, Feb 19 '19 at 18:29
You do not store an image count per car. That's why you get same number of images for every car. — Gerhardh, Feb 19 '19 at 18:32
The `pic` element is an `int *` and not an `int`, which is a bit weird. You should probably add an image count to the structure. Note that your `initCar()` function is passed an image count, but doesn't use it. — Jonathan Leffler, Feb 19 '19 at 18:37
`newCar.pic = &newImage;` This will add the same address of the same local variable each time you execute the loop. After `newCar` function is left, the memory address is no longer valid. — Gerhardh, Feb 19 '19 at 18:42
You're using the `gets()` function — don't! See [Why the `gets()` function is too dangerous to be used — ever!](https://stackoverflow.com/questions/1694036/why-is-the-gets-function-dangerous-why-should-it-not-be-used) — Jonathan Leffler, Feb 19 '19 at 18:43
You have lots of issues in your code. For example, `initCar` doesn't create memory for the pic array. You need to change it to do something like this: `Car initCar(char itsId[],char itsBrand[],int *itsPic, int imagecount){ Car newCar; strcpy(newCar.id, itsId); strcpy(newCar.brand, itsBrand); newCar.pic = malloc(imagecount * sizeof (int)); memcpy(newCar.pic, itsPic, imagecount * sizeof (int)); return newCar; } ` This way you're creating memory for the pic with `malloc`. — bruceg, Feb 19 '19 at 18:48
Your loop `while (input<1 || input<2) ;` is problematic. First the condition can be just `while (input < 2)`. Next, the loop is infinite once entered because the value of `input` never changes. — Jonathan Leffler, Feb 19 '19 at 18:54

score 0 · Answer 1 · answered Feb 19 '19 at 18:51

Your NewCar function have some problems. The newImage is in stack memory. When you do assignment newCar.pic = &newImage; the newCar.pic will point to undefined memory region because newImage was out of its scope. better way, we just use its value only, don't use address operator here. And one more thing, the newCar.pic is an pointer (array of int). So you need to allocate it before use. When you add more image item, you need to reallocate it. And initialize the pic to NULL pointer as well.

Here is my modification your NewCar function:

Car NewCar(Car minapatienter[], int patientCount)
{
    Car newCar;

    gets(newCar.id);
    printf("type in  ID \n"); 
    gets(newCar.id); 
    printf("type in brand\n");
    gets(newCar.brand);
    newCar.pic = NULL;

    bool imageInputDone = false; 
    int imageCount=0;
    while(imageInputDone == false)
    {
      printf("type in image reference \n");
      int newImage; 
      scanf("%d",&newImage);

      // Rellocation
      newCar.pic = realloc(newCar.pic, (imageCount+1)*sizeof(int));
      newCar.pic[imageCount] = newImage; 
      imageCount++;

      printf("vill du \n1.Add another image reference \n2.exit\n");
      int input;
      scanf("%d", &input);
      printf("input: %i\n",input);
      switch(input)
      {
          case 1: 
              printf("Adding one more image\n");
              break;
          case 2: 
              printf("Leaving loop\n");
              imageInputDone = true; 
              break;
          default:
              while (input<1 || input<2)
                ;
              printf("Input correct number\n");
              break;
      }

      return newCar; 
    }
}

Gerhardh · Answer 2 · 2019-02-19T19:41:47.950

You get the same number of images printed for each car because you only have a global counter. You need a counter per image:

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

#define MAX       1000
#define IDSIZE    20
#define BRANDSIZE 50
#define PICSIZE   10

typedef struct Car
{
  char  id[IDSIZE+1];
  char  brand[BRANDSIZE+1];
  int  *pic;
  int   imagecount;
} Car;

With this change there is no need to pass a count for printing:

void printCar(Car *pCar)
{
  printf(" %s ", pCar->id);
  printf("    %s ", pCar->brand);
  for(int i=0; i<pCar->imagecount; i++)
  {
    printf("   %d",pCar->pic[i]);
  }
  printf("\n");
}

The counter needs to be stored during initialization:

Car initCar(char itsId[], char itsBrand[], int *itsPic, int imagecount)
{
  Car newCar;
  strcpy(newCar.id, itsId);
  strcpy(newCar.brand, itsBrand);
  newCar.pic = itsPic;
  newCar.imagecount = imagecount;

  return newCar;
}

When you print your list, you mix up count of images and count of cars:

void PrintList(Car aLista[], int imagecount, int carcount)
{
  for(int i = 0; i<imagecount; i++)
    printCar(&aLista[i],carcount);
}

This must be:

void PrintList(Car aLista[], int carcount)
{
  for (int i = 0; i < carcount; i++)
    printCar(&aLista[i]);
}

Adding the car to your array is basically OK, but you might check if you reach MAX cars.

void AddCar(Car aList[], int *pAt, Car theCar) 
{
  aList[(*pAt)++]=theCar;    
}

Now the biggest problem. This function hads issues with memory usage and weird loops.

Car NewCar(void)
{
  Car newCar = {0};  // initialze with empty strings and NULL pointers.

  // TODO: Replace gets with fgets!
  // gets(newCar.id); // WHY read before you prompt??
  printf("type in  ID \n"); 
  gets(newCar.id); 
  printf("type in brand\n");
  gets(newCar.brand);

  bool imageInputDone = false; 
  int imageCount=0;

  while(imageInputDone == false)
  {
    printf("type in image reference \n");
    int newImage; 
    scanf("%d",&newImage);

    imageCount++;
    int *newpics = realloc(newCar.pic, (imageCount) * sizeof(int));
    newpics[imageCount-1] = newImage; 
    newCar.pic = newpics;
    // TODO: Check for NULL

    printf("vill du \n1.Add another image reference \n2.exit\n");
    int input;
    scanf("%d", &input);
    printf("input: %i\n",input);
    while (input < 1 || input > 2)
    switch(input)
    {
        case 1: 
            printf("Adding one more image\n");
            break;
        case 2: 
            printf("Leaving loop\n");
            imageInputDone = true; 
            break;
        default:
            printf("Input correct number\n");
            break;
    }

    newCar.imagecount = imageCount;
    return newCar; 
  }
}

And finally...

int main(void)
{
  int carCount=0;
  Car myCar[MAX]; 
  int input;

  int test[3]  = {1,2,3};
  int test2[3] = {4,5,6};

  myCar[0] = initCar("bbb188", "BMW", test, 3);
  myCar[1] = initCar("AAA-999", "VOLVO", test2, 3);
  myCar[2] = initCar("CCC-999", "CITROEN", test, 3);

  carCount=3;

  do
  {
    printf("1. Show cars \n2. Add car \n");
    scanf("%d", &input);

    switch(input)
    {
      case 1:
        printf("ID          BRAND         PICS \n");
        PrintList(myCar, carCount);
        break; 
      case 2: 
        AddCar(myCar, &carCount, NewCar());
        printf("ID          BRAND         PICS \n");
        PrintList(myCar, carCount);
    }    //break; 
  } while (input < 1 || input > 2); // compare as integers, not characters. Don't use < operator

  return 0;
}

The code is not tested. Remaining errors are left for exercise. ;)

If it really took days, then please regard it as a MUST DO to learn using a debugger. Running your program in GDB or similar should reveal most of the problems in a shorter time. MUCH shorter. — Gerhardh, Feb 19 '19 at 19:46

adding value to struct, adding goes wrong in C

2 Answers2