Problem with scanf to a field in a struct

Question

I've defined an array of pointers to a struct and when I try to scan to a field I get an error message and I can't understand what I did wrong.

I've tried different approaches - scanf("%s",arr[i]->code); or scanf("%s",(*(arr+i))->code); - and it still doesn't work.

Here is the beginning of my code:

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

#define N 5

typedef struct DEPARTMENT
{
    char code[11];
    int sales;
}
department;

int main()
{
    department *arr[N];
    int i;
    printf("Enter values for %d departments:", N);
    for (i = 0; i < N; i++)
    {
        printf("\nThe %d department-", (i + 1));
        printf("\nCode:");
        scanf("%s",(arr[i])->code);
        printf("\nNumber of sales:");
        scanf("%d", &((arr[i])->sales));
    }
}

Answer 1

Your immediate problem is attempting to assign values to invalid memory locations. Your declaration:

    department *arr[N];

declares an array of pointers to struct DEPARTMENT [N] (e.g. 5 pointers to struct). However each of those pointers is uninitialized and points to an indeterminate memory location. Remember, a pointer is simply a normal variable that holds the address of something else as its value, and just like a normal variable, it holds an indeterminate value until one is assigned.

Just like any other local variable declared, any attempt to access the value while the value is indeterminate results in Undefined Behavior. To use an array of pointers, each of the pointers must be assigned the starting address to a valid block of memory as its value. Here, since your intent is to provide storage for N struct, there is no need to declare N pointers and then allocate storage for N struct independently. You can simply declare a pointer to struct and then allocate storage for N struct in a single block of memory, e.g.

#define N 5
...
typedef struct {
    char code[MAXC];
    int sales;
} department;
...
    department *arr;        /* declares a pointer to struct */
    ...
    /* allocate/validate storage for N struct */
    if ((arr = malloc (N * sizeof *arr)) == NULL) {
        perror ("malloc-arr");
        return 1;
    }

(note: always validate every allocation)

Allocating storage for N struct in a single block has the advantage of providing a single free() to release the allocated block of memory.

Just as you must validate every allocation, you must validate every user input. That means at minimum, you must validate the return of each all to scanf. However, your use of scanf has a downside. Input with scanf is horribly fragile as any variation in input will lead to a matching failure, character extraction from stdin will cease at the point the matching failure occurs leaving the offending character in stdin unread just waiting to bite you again on your next call to scanf. Further, if there are any inadvertent characters that follow the valid input, they too remain in stdin unread.

Your options are to empty stdin after each input to ensure no offending characters remain, or, the better option is to read a complete line of input each time with a line-oriented input function like fgets() or POSIX getline() and then parse the value you need from the filled buffer. This has many benefits. Any extraneous characters are read and discarded with each input. You also benefit from being able to independently validate (1) the read; and (2) the parse of needed information from the buffer. You can use sscanf to parse information from the filled buffer just as you would use scanf to read from stdin.

Putting that altogether, you could rewrite your code something like the following:

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

#define N 5
#define CODESZ 12   /* if you need more than 1 constant, define them */
#define MAXC 1024   /* (don't skimp on buffer size) */

typedef struct {
    char code[MAXC];
    int sales;
} department;

int main()
{
    department *arr;        /* declares a pointer to struct */
    char buf[MAXC];         /* buffer to hold each line */
    int i, ndx = 0;

    /* allocate/validate storage for N struct */
    if ((arr = malloc (N * sizeof *arr)) == NULL) {
        perror ("malloc-arr");
        return 1;
    }

    printf("Enter values for %d departments:\n", N);
    while (ndx < N) {       /* loop until info for N departments received */
        printf ("\nThe %d department-\n  Code  : ", ndx + 1);
        if (fgets (buf, MAXC, stdin) == NULL || 
                sscanf (buf, "%11s", arr[ndx].code) != 1)
            break;
        fputs ("  Sales : ", stdout);
        if (fgets (buf, MAXC, stdin) == NULL ||
                sscanf (buf, "%d", &arr[ndx].sales) != 1)
            break;
        ndx++;
    }
    puts ("\nDepartment Sales Results:\n");
    for (i = 0; i < ndx; i++)   /* output results, free memory */
        printf ("Dept Code: %-12s   Sales: %d\n", arr[i].code, arr[i].sales);

    free (arr); /* don't forget to free what you allocate */
}

(note: a separate index counter ndx is used which provides a count of the actual number of structs filled in the even the user cancels input after entering, e.g. 3 departments instead of 5)

Example Use/Output

$ ./bin/allocstructloop
Enter values for 5 departments:

The 1 department-
  Code  : 001
  Sales : 123

The 2 department-
  Code  : 002
  Sales : 234

The 3 department-
  Code  : 003 -- this department met sales goals.
  Sales : 345

The 4 department-
  Code  : 004
  Sales : 456 -- this department exceeded sales goals.

The 5 department-
  Code  : 005 -- this department had most sales for period.
  Sales : 567

Department Sales Results:

Dept Code: 001            Sales: 123
Dept Code: 002            Sales: 234
Dept Code: 003            Sales: 345
Dept Code: 004            Sales: 456
Dept Code: 005            Sales: 567

Try entering the additional text (or even having a slip of an extra keystroke) after your input and see how your code responds. Keep Ctrl+C on the ready.

Look things over and let me know if you have additional questions.

Answer 2

Altough you did declare your department array, you didnt allocte the memory of each department.

You could do this inside your loop, where you are filling your array:

for (i = 0; i < N; i++)
{
    arr[i] = malloc(sizeof(department));
    /* .. */
}

A cleaner solution as mentioned in the comments, one allocation should be enough:

department *arr = malloc(sizeof(department) * N);

Dont forget to free the allocated memory and check for mallocs return value.