C structures displaying numbers rather than user data

Question

Kindly help me debug this code. It is not displaying the correct data. The following program is supposed to get book details from the user, dynamically allocate memory to them and display them.

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

int main()
{
    struct books *b;
    b = (struct books*)malloc(sizeof(struct books));
    int command, flag = 0;
    int n=0, i;

    while(flag == 0)
    {
        printf ("1. Add Book\n");
        printf ("2. View Books\n");
        printf ("3. Quit\n");
        scanf("%d", &command);

        if (command == 1)
        {
            printf ("Enter Name\n");
            //scanf("%d", &(b+i)->name);
            scanf(" ");
            gets((b+i)->name);
            printf ("Enter Author\n");
            //scanf("%d", &(b+i)->author);
            scanf(" ");
            gets((b+i)->author);
            printf ("Enter Year Published\n");
            scanf("%d", &(b+i)->year_published);
            n=n+1;
            i=n;
        } else if (command == 2)
        {
            for(i=0; i<n; i++)
            {
                printf ("%d - %d by %d\n", (b+i)->year_published, (b+i)->name, (b+i)->author);
            }
        } else if (command == 3)
        {
            flag = 1;
        } else
        {
            printf ("Invalid choice!\n");
        }
    }
}

The following is problem5.h header file that has the structure books. Initially I didn't declare the variables in array since I didn't want to use much memory. But I had to due to many errors.

#define PROBLEM3_H_INCLUDED

typedef struct books{
    char *name[30];
    char *author[30];
    int year_published;
};

#endif // PROBLEM3_H_INCLUDED

When I print I am getting random numbers instead of the data the user entered.

use `%s` to print `char *`, not `%d`. You can read compiler warning too — Cid, Nov 17 '20 at 08:29
To add to Cids comment, use %s to **read** `char *` as well. Better yet, use `fgets()` to avoid problems with books / authors that don't have one-word names / titles (i.e., all of them). — DevSolar, Nov 17 '20 at 08:32
Also, in C you [shouldn't really cast the result of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — Some programmer dude, Nov 17 '20 at 08:34
And never ***ever*** use `gets`. It's [a dangerous function](https://stackoverflow.com/questions/1694036/why-is-the-gets-function-so-dangerous-that-it-should-not-be-used) and should be avoided at all cost. Use e.g. [`fgets`](https://en.cppreference.com/w/c/io/fgets) instead. — Some programmer dude, Nov 17 '20 at 08:35
Oh, and instead of e.g. `(b+i)->name`, why not the more readable and understandable `b[i].name`? — Some programmer dude, Nov 17 '20 at 08:36

Jabberwocky · Accepted Answer · 2020-11-17T09:04:59.047

The overall design of your code is wrong.

This is basically what you want.

I made following changements:

using meaningful variable names
changed struct book so the structure can contain one book. Also renamed it from struct books to struct book because the structure contains only one book.
allocating memory properly
using books[numberofbooks].x instead of the less readable *(books + numberofbooks)->x

More explanations in the comments.

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

struct book {
  char name[30];
  char author[30];
  int year_published;
};

int main()
{
  struct book* books = NULL;     // no books at all initially so we
                                 // initialize to NULL
                                 // so we can simply use realloc
  int numberofbooks = 0;
  int programend = 0;

  while (programend == 0)
  {
    printf("1. Add Book\n");
    printf("2. View Books\n");
    printf("3. Quit\n");
    int command;
    scanf("%d", &command);

    if (command == 1)
    {
      getchar();   // consume Enter key (due su scanf)

      // allocate memory for one more book
      books = realloc(books, sizeof(struct book) * (numberofbooks + 1));

      printf("Enter Name\n");
      gets(books[numberofbooks].name);

      printf("Enter Author\n");
      gets(books[numberofbooks].author);

      printf("Enter Year Published\n");
      scanf("%d", &books[numberofbooks].year_published);

      numberofbooks++;   // increment number of books
    }
    else if (command == 2)
    {
      for (int i = 0; i < numberofbooks; i++)
      {
        printf("%d - %s by %s\n", books[i].year_published, books[i].name, books[i].author);
      }
    }
    else if (command == 3)
    {
      programend = 1;
    }
    else
    {
      printf("Invalid choice!\n");
    }
  }
}

There is still room for improvement though:

error checking for realloc
error checking for interactive I/O
not using the deprecated and dangerous gets
and certainly a few other things

Great answer. Only point I don't agree: "not using the deprecated and dangerous gets" at least on macOS, you may think you are using `gets` or `strcpy` when actually the compiler / linker switched out unsafe C APIs. I have found no way to compile on macOS with `clang` that allows unsafe C APIs. — rustyMagnet, Nov 17 '20 at 14:53
@rustyMagnet I mentioned this in the "room for improvement" section at the end of the answer. I left `gets` to stay as close as possible to the original code. — Jabberwocky, Nov 17 '20 at 14:54

score 0 · Answer 2 · answered Nov 17 '20 at 08:46

b = (struct books*)malloc(sizeof(struct books));

Here, you are allocating memory for only one instance of struct books , But you are accessing multiple instances of struct books.

printf ("%d - %d by %d\n", (b+i)->year_published, (b+i)->name, (b+i)->author);

For i>=1 (b+i) is not defined, because you did not allocate memory for it. You have allocated memory for only (b+0).

int n=0, i;
gets((b+i)->name);

Here, i has not been initiliazed.

C structures displaying numbers rather than user data

2 Answers2