c gets/fgets isnt working

Question

gets doesn't work in the function neuePerson, it worked when it was in a for loop, but then I changed it and now the compiler says isn't undefined.

I tried it with fgets, now there is no warning, but it still ignores fgets, so I cant write anything in the console.

in the main function's gets works. I'm a little bit confused... :o

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include "readline.h"

//typedef struct Person {
//    char name[50];
//    char unit;
//    int number;
//} Person;

typedef struct person {
    char name[50];
    char unit;
    int number;
    struct person *next;
} Person;

void neuePerson(Person *firstPerson) {
    time_t t; 
    time(&t);
    srand((unsigned int)t);
    while (firstPerson->next != 0)
        firstPerson = firstPerson->next;
    printf("Gib  einen Namen ein \n");
    fgets(firstPerson->name, 50, stdin);                        
    firstPerson->number = rand() % 99 + 1; 
    firstPerson->unit = rand() % 3 + 65;
    firstPerson->next = (Person*)malloc(sizeof(Person));
    firstPerson = firstPerson->next;
    firstPerson->next = 0;
}

void ausgabe(Person *anfang) {
    while (anfang->next != 0) {
        printf("Name: %s", anfang->name);
        printf("   Abteilung: %c", anfang->unit);
        printf("   Tel.Nummer: %i\n", anfang->number);
        anfang = anfang->next;
    }
}

int main() {
    Person* pers1 = (Person*)malloc(sizeof(Person));
    //Person* test = (Person*)malloc(sizeof(Person));
    //gets(test->name, 50);
    //printf("%s", test->name);
    pers1->next = 0;
    char z = 'n';
    while (z != 'e') {
        printf("[n]eue Person, [a]usgabe,  [e]nde");
        z = getchar();
        if (z == 'n') neuePerson(pers1);
        else if (z == 'a') ausgabe(pers1);
    }
}

Answer 1

The problem comes from the line buffering of standard input:

You read the option in main with getchar(), but the byte is returned to your program after you type the enter key. Only the initial char from the line is returned, the rest stays in the stream.

When you subsequently read the person's name with fgets(), it returns an empty line because it gets the \n that is still in the stream. Contrary to popular belief, fflush(stdin) is not the solution because it has undefined behavior. A better solution is to read the option this way:

int main() {
    Person *pers1 = (Person*)malloc(sizeof(Person));
    pers1->next = NULL;
    pers1->unit = 0;
    pers1->name[0] = '\0';
    for (;;) {
        int z, c;
        printf("[n]eue Person, [a]usgabe,  [e]nde ");
        z = c = getchar();
        while (c != EOF && c != '\n')
            c = getchar();
        if (z == EOF || z == 'e')
            break;
        else
        if (z == 'n')
            neuePerson(pers1);
        else
        if (z == 'a')
            ausgabe(pers1);
    }
}

You should improve your list handling: an empty list should be just NULL, it is incorrect to keep a dummy uninitialized structure pending at the end of the list. You can handle the update to the list head by passing a pointer to the head to neuePerson.

Answer 2

I agree with chqrlie's answer; in addition, don't forget to free up your list at after you exit the main while loop:

int main()
{
   /** your While loop */

   Person *nextp = pers1;
   do {
       free(nextp);
       nextp = nextp->next;
   } while (nextp != NULL);
}

It would be a good idea to separate the linked-list logic from everything else. You'll be glad you did now and when your program becomes larger.

Also, Become friends with valgrind.

Answer 3

First, since you asked about both gets and fgets, that enables me to quote from the man page :

Never use gets(). Because it is impossible to tell without knowing the data in advance how many characters gets() will read, and because gets() will continue to store characters past the end of the buffer, it is extremely dangerous to use. It has been used to break computer security. Use fgets() instead.

I am going to take the liberty of re-writing your code to the minimal set, before I answer your question. You're testing gets, so I can remove everything afterward, and everything in your code that isn't called before gets. I'll also move your call to fgets from neuePerson to main. I'll also avoid heap memory to simply further, I trust you to figure out how to use the heap properly. Finally, I really don't like using uninitialized structs or main routines that don't have exit codes, so I'll do that as well. That looks like this :

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include "readline.h"

typedef struct person{
    char name[50];
    char unit;
    int number;
    struct person* next;
} Person;

int main() {
    Person _pers1, *pers1 = &_pers1;
    char z = 'n';

    memset(pers1, 0, sizeof(Person));

    while (z != 'e') {
        z = getchar();
        pers1->name = fgets(pers1->name, 50, stdin);
    }

    return 0;
}

At a high level, the problem is that you have two methods that handle strings in different ways. You've already been shown a solution that takes one of the methods - getchar - and makes it work like another method - fgets with a buffer size of 1 in this case. But there may well be many situations where you do not have enough information about both methods to do this. In this situation, for instance, if you did not know that newline characters were in the input feed at all, or you were programming in a language where fgets had a programmable stop rather than just stopping on a newline character, your original approach may have been more sensible.

So in this situation, when two methods aren't cooperating, it's often a good idea to use the same method throughout. That looks like this :

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include "readline.h"

typedef struct person{
    char name[50];
    char unit;
    int number;
    struct person* next;
} Person;

int main() {
    Person _pers1, *pers1 = &_pers1;
    char z[50];

    memset(pers1, 0, sizeof(Person));
    memset(z, 0, sizeof(char) * 50);

    while (z[0] != 'e') {
        fgets(z, 50, stdin);
        fgets(pers1->name, 50, stdin);
    }

    return 0;
}

Making z 50 bytes big is of course overkill. I did this to illustrate a principle. If you use the same method the same way everywhere, you won't run into problems. You don't need to ask questions like "wait does z need to be 1 or 2 bytes? Should I call fgets with 2 or 1?". You already know "50 is the most I'll allow input to be". You can come back and optimize later, if there ends up being a reason to optimize.

I also want to mention, it's true that this line,

    while (z[0] != 'e') {

has some flaws. It would be more correct to look at values other than 'e' . I'd recommend 0, EOF, '\n', and '\r'. But the only one of those you could have known about in advance is 0, since you set that. It would be best, in my opinion, to discover that the others needed to be added through testing and using your code, rather than to "kitchen sink" your code to avoid problems before they happen.