Home > Blockchain >  Why is my nested array of Structs overwriting previously stored values?
Why is my nested array of Structs overwriting previously stored values?

Time:12-29

So basically here I am just seeing whether my previously stored input, that is temp->voterDetails[0].name, has its value restored after another input.
Unfortunately it does not seem to store its value. Could you help me understanding why?

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


struct Voter {
    char *name;
};

struct Block{
    // Creating a block containing 5 voter details
    struct Voter voterDetails[5];
};

int main() {
    int choice;
    struct Block *Genesis = (struct Block *)malloc(sizeof(struct Block));
    struct Block *temp = Genesis;
    struct Block *newnode = (struct Block *)malloc(sizeof(struct Block));
    char *nm;
    int voteNum = 0;
    int blockNumber = 1;
    do{
        printf("What do you want to do\n1. Cast a vote\n2. Check your vote\n3. Exit\n");
        scanf("%d", &choice);
    
        switch(choice){
            case 1:
                printf("Enter name");
                fgets(nm, 30, stdin);
                fgets(nm, 30, stdin);
                voteNum  = 1;
                temp->voterDetails[voteNum-1].name = nm;
                puts(Genesis->voterDetails[0].name);

                break;
        }
    } while (choice != 3);
    return 0;
}

CodePudding user response:

At least these problems:

Uninitialized/assign value

Calling fgets(nm, 30, stdin); yet nm is not yet assigned. UB

CodePudding user response:

Your program has a lot of issues:

  1. nm temporary pointer (that in your snippet isn't even necessary, but I assume it can be useful in the whole program) is not initialized. It should be allocated, instead
  2. Each element of the array temp->voterDetails[voteNum].name has to be allocated as well
  3. Strings are copied using strcpy function
  4. Whenever you perform a choice the newline remains in the input buffer, making the first fgets return immediately with no data. That's what you "solved" calling fgets twice, but it's not a real solution
  5. Data read with fgets will have trailing newline, that will have to be removed from the name buffer

Here it is the working code:

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

#define MAX_VOTERS 5

struct Voter {
    char *name;
};

struct Block{
    // Creating a block containing 5 voter details
    struct Voter voterDetails[MAX_VOTERS];
};

void flushInput(void)
{
    int c;
    while ((c = getchar()) != '\n' && c != EOF) { }
}

int main() {
    int choice;
    struct Block *Genesis = (struct Block *)malloc(sizeof(struct Block));
    struct Block *temp = Genesis;
    struct Block *newnode = (struct Block *)malloc(sizeof(struct Block));
    char *nm = malloc(30 * sizeof(char)); // nm temporary pointer allocation
    int voteNum = 0;
    int blockNumber = 1;
    do{
        printf("What do you want to do\n1. Cast a vote\n2. Check your vote\n3. Exit\n");
        scanf("%d", &choice);
        flushInput();
        switch(choice){
            case 1:
                printf("Enter name\n"); // Better with a newline
                fgets(nm, 30, stdin);   // Useless extra fgets removed
                nm[strcspn(nm, "\n")] = 0;
                temp->voterDetails[voteNum].name = malloc(30*sizeof(char));
                strcpy(temp->voterDetails[voteNum].name, nm);   // Strings cannot be assigned, but are copied with strcpy
                                                                // voteNum incremented later, so '-1' can be avoided. Isn't it more readable?
                printf("Name: %s..\n", Genesis->voterDetails[voteNum].name); // printf used instead of puts, in order to append a newline
                voteNum  = 1;
                
                break;
        }
    } while (choice != 3 && voteNum < MAX_VOTERS); // The program is safer if you check that votes don't exceed the number of location in your array

    free(nm);
    /* ... remember to free all the remaining pointers as soon as you don't need them anymore */
    return 0;
}

Output:

What do you want to do
1. Cast a vote
2. Check your vote
3. Exit
1
Enter name
Roberto
Name: Roberto
What do you want to do
1. Cast a vote
2. Check your vote
3. Exit
...

Some extra comments:

  1. Pending data in stdin is flushed using the custom function flushInput() (consult this answer for more details)
  2. After flushing correctly the input buffer, the extra fgets can be removed
  3. It's always better to make sure that the user doesn't insert data we don't have room for. In this case, since the voterDetails array has 5 elements, we make sure to exit the loop as soon as the fifth name has been inserted
  4. Since the constant '5' is now used several times, it is better to avoid using "magic numbers" in the code. Just define a precompiler constant, instead (MAX_VOTERS)

CodePudding user response:

You're not allocating any space for your pointers. This doesn't happen "automatically" in C, you have to manually do it. From your fgets argument of 30, it seems you want to accept names with a max of 30 chars. If that's the case, then the easiest thing to do is simply use char arrays instead of pointers:

struct Voter {
    char name[30]; // note because of the required NUL terminator, this
                   // will only fit 29 printable characters
};

and the same for nm

char nm[30] = { 0 };  // initializes the array to 0s, altho fgets
                      // will always add a NUL terminator

But even with those fixes, you still need to copy the string data entered in nm to your names:

#include <string.h>
...
strcpy(temp->voterDetails[voteNum-1].name, nm);

Or IMHO, there's no point in using the temp nm anyway, just plug temp->voterDetails[voteNum].name directly into fgets. Also note that fgets will add a newline to the array if you don't max out the buffer size. You can search for ways to remove that if you don't want it.

If you want to use pointers instead of arrays, you must malloc space for name and nm as you have for your struct Block pointers.

  • Related