0

I'm learning about C and having trouble with this. It does compile but the result is unexpected. In my code, I have a struct:

typedef struct {
    char *title[50];
    float price;
} Book;

In the main, I am asking the user for number of books they want to have in the library. Then let them initialize the attributes of each book. Finally, print them out to the terminal using Display function.

void Display(Book* lib, int n);

int main() {
    int n;
    printf("Enter the number of books:\n" );
    scanf("%d", &n);
    if(n <= 0) {
        printf("Invalid number of book.");
        return 1;
    }

    Book *lib = (Book *) malloc(n * sizeof(Book));
    if(lib == NULL) {
        printf("The memory is full");
        return 1;
    }

    for(int i = 0; i < n; ++i) {
        char title[50];
        float price;
        printf("Book no.%d\n", i);
        printf("Book Title: ");
        scanf("%s", title);
        printf("Book Price: ");
        scanf("%f", &price);
        printf("\n");
        *(lib+i)->title = title;
        (lib+i)->price = price;
    }

    Display(lib, n);

    return 0;
}

The code compiles successfully, but the result is like this:

Enter the number of books:
2
Book no.0
Book Title: AAAA
Book Price: 1.0

Book no.1
Book Title: BBBB
Book Price: 9.9


----Displaying----
Book no.0
Book Title: BBBB
Book Price: $1.000000

Book no.1
Book Title: BBBB
Book Price: $9.900000

The title of the first book is wrong and it is the title of the second book. Why does this happen? And how should I fix it? Thank you

Edit: One of the requirements in my assignment is the title of Book must be of type char*

Edit 2: I realized my mistake when having 50 pointers of type char now. How should I fix it?

Improve this question
4
  • 2
    char *title[50]; that's not a string pointer; that's 50 string pointers. Commented Jun 16, 2020 at 19:39
  • I'm not sure how this works. I thought string is an array of char in C, that's why I have char *title[50]. Should I allocate it by malloc? Commented Jun 16, 2020 at 19:42
  • why do you need the char title[50]; at all. you have allocated in the heap scanf to the heap no need for this char array and fix the title to be char title[50 not char* title[50] in the struct definition Commented Jun 16, 2020 at 19:45
  • Thank you all. But having title of type char* is actually one of the requirements in my assignment. Commented Jun 16, 2020 at 19:48

3 Answers 3

Reset to default
4

In your struct defintion:

typedef struct {
    char *title[50];
    float price;
} Book;

You don't have an array of char (which can hold a string) but an array of pointers to char each of which can point to a string.

Also, this doesn't do what you expect due to the definition of the price member:

*(lib+i)->title = title;

Change the definition to:

typedef struct {
    char title[50];
    float price;
} Book;

And read directly into the struct fields instead of temp variables to avoid copying:

    printf("Book Title: ");
    scanf("%s", lib[i].title);
    printf("Book Price: ");
    scanf("%f", &lib[i].price);

Alternately, you can define title as a char *:

typedef struct {
    char *title;
    float price;
} Book;

In which case you have to allocate space for the pointer to point to:

    lib[i].title = malloc(50);
    printf("Book Title: ");
    scanf("%s", lib[i].title);

Note that you can't have it point to a local like you did before because that local goes out of scope at the end of the loop, making the pointer invalid.

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

Thank you for your response. But one of the requirements in my assignment is Book must have a title of type char.
@Duke_ngn I think you're misunderstanding your requirement. A string is stored in a char array, for example char title[50]. That's what the code above has.
I am sorry. I mistyped. The requirement of my assignment is title is of type char*. Like we have to use a pointer here
Thank you. It works now! I'm just curious but can you explain why my original code produces that result instead of crashing.
@Duke_ngn Each book's title was pointing to the same local variable in the for loop. You were "lucky" that the memory used by that variable wasn't overwritten with something else when you printed the list. Just because the code could crash doesn't mean it will.
|
2

If you have to use a pointer, you'll need to use dynamic allocation.

The structure member should be declared as a pointer, not an array of pointers:

typedef struct {
    char *title;
    float price;
} Book;

Then the loop should allocate memory for a copy of the title, and copy the title into it.

    for(int i = 0; i < n; ++i) {
        char title[50];
        float price;
        printf("Book no.%d\n", i);
        printf("Book Title: ");
        scanf("%s", title);
        printf("Book Price: ");
        scanf("%f", &price);
        printf("\n");
        (lib+i)->title = malloc(strlen(title)+1); // +1 for the trailing null
        strcpy((lib+i)->title, title);
        (lib+i)->price = price;
    }
Improve this answer

Comments

-1

Before you updated your post the declaration of the data member title of the structure

typedef struct {
    char *title[50];
    float price;
} Book;

is incorrect (in the context of the program it does not make sense). The structure should look like

typedef struct {
    char title[50];
    float price;
} Book;

That is each book has one title and not 50 pointers to titles.

And instead of this statement

*(lib+i)->title = title;

you have to write at least like

#include <string.h>

//...

strcpy( ( lib+i )->title, title );

As for your output then you assigned the address of the same local variable title to the first pointer of the array

    char *title[50];

of each element of the dynamically allocated array.

Pay attention to that you should free the dynamically allocated memory when it is not used any more. For example

free( lib );

Edit: One of the requirements in my assignment is the title of Book must be of type char*

After you updated your post then in this case the structure definition should look like

typedef struct {
    char *title;
    float price;
} Book;

And you will need to allocate dynamically memory for entered title of an object of the structure.

You could do it for example the following way. I suppose that you want to use pointers instead of the subscript operator.

( lib + i )->title = malloc( strlen( title ) + 1 );
strcpy( ( lib + i )->title, title );

In this case before freeing the allocated array pointed to by the pointer lib you will need to free also each allocated character array like for example

for ( int i = 0; i < n; i++ )
{
    free( ( lib + i )->title );
}

free( lib );
Improve this answer

11 Comments

why he need to use the strcpy at all?? scanf directly to the heap (to the lib allocated array) no need for this
@Adam I did not say that he needs. But he uses the local variable title to enter a string.
cos he is begginer and u should correct and lead him! :-)
@Adam I see nothing wrong to use a local variable for this purpose.
@Duke_ngn One more byte is required if you want to store a string that includes the terminating zero character '\0'. Otherwise the array will not contain a string that will create some difficulties to output it instead of for example simple puts( ( lib + i )->title );
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.