C Program terminates after if/else or repeats if I use fputs/fgets

Question

I am very new to C and have dabbled in Objective-C, AppleScript, and HTML/CSS. I'm sure that my problem is very easy to solve. I am trying to write something that will allow me to input source data and have it ordered in a certain way as output (in this case, citations). Basically, I want to save name, title, publisher, etc. as variables and print them in a certain order.

Here's the issue: The code here terminates too early and when I use fputs and fgets with stdout and stdin it gets stuck and asks the same question forever. What am I missing?

int source_type;
int NumberofAuthors;
char AuthorName1[20];
char AuthorName2[20];
char AuthorName3[20];
char title[20];
char url[100];
char publishingCity[20];
char publisher[20];
char yearPublished[20];
char pageNumbers[20];
int valid;

printf("Welcome to Jackson's Chicago Manual of Style Auto-Footnoter.\n");

fputs("Choose source type:\n a.Book\n b.Journal\n c.Article\n d.Website\n ", stdout);
source_type = getchar();

if (source_type == 'a') {
    valid = 1;
} else {
    printf("Invalid source selection");
}

while ( valid == 1 && source_type == 'a' )
{
    printf("Number of authors [1 or 2]: ");
    scanf( "%d", &NumberofAuthors);
    if ( NumberofAuthors > 0 && NumberofAuthors < 3 ) {
        valid = 1;
        printf("Got it, %d author(s).\n", NumberofAuthors);
    }
    else {
        printf( "That's not enough people to write a book.\n" );
    }

    if ( NumberofAuthors == 1 ) {
        printf( "Author's name: " );
        scanf("%c", &AuthorName1);

    } 
    if (NumberofAuthors == 2) {
        printf("First author's name: " );
        scanf("%c", &AuthorName2);
        printf("Second author's name: " );
        scanf("%c", &AuthorName3);
    }
    else {
        valid = 0;
    }

    printf("Book title: " );
    fgets(title, sizeof(title), stdin);

    printf("Publication city: " );
    fgets(publishingCity, sizeof(publishingCity), stdin);


    } 


return 0;

First of all, when fetching strings with `scanf`, use `%s` instead, and you don't need the `&` for the char-arrays. — Some programmer dude, Nov 04 '11 at 23:50
Then there is something else wrong, as the code you supposedly changed is all inside the loop and doesn't have anything to do with the loop condition. Also, is there a reason you are using `fgets` to get some strings, but not for others? — Some programmer dude, Nov 05 '11 at 00:07
Honestly, the `fgets` thing is because they were all originally as they are here, then I read that I shouldn't use `scanf`, so I changed it to `fgets` and the whole thing was useless. I changed it back (except for the few you see) and have been dealing with the hang-ups since. Taking another look at the `&`'s. — Sneagan, Nov 05 '11 at 00:09
Took out the `&`'s again and got a bad access signal at `scanf( "%d", NumberofAuthors);` — Sneagan, Nov 05 '11 at 00:11
Only remove the `&` on the strings you get with `scanf`. That is because they are already pointers. When you do `scanf` on single characters (`%c`) or integers (`%d`) then you need the address of the variable to store the value, with strings (`char *` or `char x[] = ".."`) then the variable itself already is an address. — Some programmer dude, Nov 05 '11 at 00:16
aaAAAAaaah. Thanks! Any ideas about why it's stopping at the `if ( NumberofAuthors == 1 ) {` line? — Sneagan, Nov 05 '11 at 00:19

score 1 · Answer 1 · answered Nov 05 '11 at 00:30

On the beggining of the program:

if (source_type == 'a') {
    valid = 1;
} else {
    printf("Invalid source selection");
}

In case source_type is invalid, valid still contains garbage value, and using an uninitialized variable is undefined behaviour. Moving on.

while ( valid == 1 && source_type == 'a' )
{
    printf("Number of authors [1 or 2]: ");
    scanf( "%d", &NumberofAuthors);
    if ( NumberofAuthors > 0 && NumberofAuthors < 3 ) {
        valid = 1;
        printf("Got it, %d author(s).\n", NumberofAuthors);
    }
    //...

You never reset valid to 0. You should consider using a switch() for that part of the while loop. It makes it more easy to read.

Also

if ( NumberofAuthors == 1 ) {
    printf( "Author's name: " );
    scanf("%c", &AuthorName1);

} 
if (NumberofAuthors == 2) {
    printf("First author's name: " );
    scanf("%c", &AuthorName2);
    printf("Second author's name: " );
    scanf("%c", &AuthorName3);
}
else {
    valid = 0;
}

I hope you realize that incase NumberofAuthors == 1 the else part is going to executed and set valid = 0. That is because the else sticks on just the closest if, and only that.

I guess you use fgets etc to avoid overflows. Good. See that trick on the scanfs. Read more here : http://www.cplusplus.com/reference/clibrary/cstdio/scanf/

Try that:

int main(int argc, char* argv[])
{
    char source_type;
    int NumberofAuthors;
    char AuthorName1[20];
    char AuthorName2[20];
    char AuthorName3[20];
    char title[20];
    char url[100];
    char publishingCity[20];
    char publisher[20];
    char yearPublished[20];
    char pageNumbers[20];
    int valid;

    printf("Welcome to Jackson's Chicago Manual of Style Auto-Footnoter.\n");

    printf("Choose source type:\n a.Book");
    scanf("%c" , &source_type);

    if (source_type == 'a') {
        valid = 1;
    } else {
        printf("Invalid source selection");
        valid = 0;
    }

    while ( valid == 1 && source_type == 'a' )
    {
        //Reset
        valid = 0;

        printf("Number of authors [1 or 2]: ");
        scanf( "%d", &NumberofAuthors);
        if ( NumberofAuthors > 0 && NumberofAuthors < 3 ) {
            valid = 1;
            printf("Got it, %d author(s).\n", NumberofAuthors);
        }
        else {
            printf( "That's not enough people to write a book.\n" );
            continue;
        }

        switch( NumberofAuthors )
        {
        case 1:
            printf( "Author's name: " );
            scanf("%19s", AuthorName1);
            break;

        case 2:
            printf("First author's name: " );
            scanf("%19s", AuthorName2);
            printf("Second author's name: " );
            scanf("%19s", AuthorName3);
            break;

        default:
            valid = 0;
            break;
        }

        if(valid)
        {
            printf("Book title: " );
            scanf("%19s" , title);

            printf("Publication city: " );
            scanf("%19s" , publishingCity );
        }

    } 
    return 0;
}

Forgot to mention: Notice the `continue` in the first `if` in the while loop. — , Nov 05 '11 at 00:37

score 0 · Answer 2 · answered Nov 04 '11 at 23:48

You only change source_type outside of the while loop, so once entered the loop the only way to get out is by assigning 0 to valid. This is done every time NumberofAuthors != 2, since the first if is not within the if-else chain. Perhaps you want this instead:

if ( NumberofAuthors == 1 ) {
    printf( "Author's name: " );
    scanf("%c", &AuthorName1);
} else if (NumberofAuthors == 2) {
    printf("First author's name: " );
    scanf("%c", &AuthorName2);
    printf("Second author's name: " );
    scanf("%c", &AuthorName3);
} else {
    valid = 0;
}

It's still quitting as soon as it encounters the the first `if` within `while`. :/ — Sneagan, Nov 05 '11 at 00:03

score 0 · Answer 3 · answered Nov 05 '11 at 01:16

You are using %c to read names; this is not good. You're passing the address of an array, rather than the pointer to the first element of the array; this is also not good. You are trampling all over the place, and leaving yourself with undefined behaviour problems.

The 'obvious' fix is to use %s and forego the & in front of the array names, but you must not succumb to the obvious as it is wrong. Most authors have a space between the first and last name (or initials and surname), and %s stops at the first space. You need to use fgets() to read the names, but remember to remove the trailing newline.

It is not quite clear to me why you have a single author's name in 'AuthorName1' but double authors go in 'AuthorName2' and 'AuthorName3'; I'd expect to use 'name 1' for the first author regardless. Indeed, it might be better to have an array of author names - that generalizes more readily.

When I compile your code (wrapping it in int main(void) { and } and including <stdio.h>, I get compilation warnings:

xx.c: In function ‘main’:
xx.c:43: warning: format ‘%c’ expects type ‘char *’, but argument 2 has type ‘char (*)[20]’
xx.c:43: warning: format ‘%c’ expects type ‘char *’, but argument 2 has type ‘char (*)[20]’
xx.c:48: warning: format ‘%c’ expects type ‘char *’, but argument 2 has type ‘char (*)[20]’
xx.c:48: warning: format ‘%c’ expects type ‘char *’, but argument 2 has type ‘char (*)[20]’
xx.c:50: warning: format ‘%c’ expects type ‘char *’, but argument 2 has type ‘char (*)[20]’
xx.c:50: warning: format ‘%c’ expects type ‘char *’, but argument 2 has type ‘char (*)[20]’

I rewrote the code to use a function to handle the prompting for and reading of strings, thus:

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

static int get_string(const char *prompt, char *buffer, size_t bufsiz)
{
    char *nl;
    printf("%s: ", prompt);
    fflush(0);
    if (fgets(buffer, bufsiz, stdin) == 0)
        return EOF; /* Read error - EOF */
    if ((nl = strchr(buffer, '\n')) == 0)
    {
        fprintf(stderr, "Overlong string entered!\n");
        return EOF;
    }
    *nl = '\0';
    return 0;
}

int main(void)
{
    int source_type;
    int NumberofAuthors;
    char AuthorName1[20];
    char AuthorName2[20];
    char title[20];
    char publishingCity[20];
    int valid = 0;
    int c;

    printf("Welcome to Jackson's Chicago Manual of Style Auto-Footnoter.\n");

    fputs("Choose source type:\n a.Book\n b.Journal\n c.Article\n d.Website\n ", stdout);
    source_type = getchar();

    if (source_type == 'a')
        valid = 1;
    else
    {
        printf("Invalid source selection");
        return(1);
    }

    /* Lucky that scanf() skips over the newline in search of digits! */
    while (valid == 1 && source_type == 'a')
    {
        printf("Number of authors [1 or 2]");
        scanf("%d", &NumberofAuthors);
        if (NumberofAuthors > 0 && NumberofAuthors < 3)
        {
            valid = 1;
            printf("Got it, %d author(s).\n", NumberofAuthors);
        }
        else
        {
            printf("That's not enough (or too many) people to write a book.\n");
            break;
        }
        /* Gobble to newline */
        while ((c = getchar()) != EOF && c != '\n')
            ;

        if (NumberofAuthors == 1)
        {
            if (get_string("Author's name", AuthorName1, sizeof(AuthorName1)) == EOF)
            {
                valid = 0;
                break;
            }
        } 
        else
        {
            assert(NumberofAuthors == 2);
            if (get_string("First author's name",  AuthorName1, sizeof(AuthorName1)) == EOF ||
                get_string("Second author's name", AuthorName2, sizeof(AuthorName2)) == EOF)
            {
                valid = 0;
                break;
            }
        }

        if (get_string("Book title", title, sizeof(title)) == EOF ||
            get_string("Publication city", publishingCity, sizeof(publishingCity)) == EOF)
        {
            valid = 0;
            break;
        }

        printf("Author 1: %s\n", AuthorName1);
        if (NumberofAuthors == 2)
            printf("Author 2: %s\n", AuthorName2);
        printf("Book title: %s\n", title);
        printf("Publication city: %s\n", publishingCity);
    } 

    return 0;
}

I kept the 'valid' flag, but it really doesn't pay for itself. The code is simpler and shorter without it.

Thanks so much. I realize that this is obviously very sloppy work to a trained eye, but I'm really trying to get the hang of it. This is my first foray into writing a program from scratch and it seems I've messed up a lot. You and the others who have responded are really great and I appreciate the time you have all put into helping me. Hopefully my next submission will be less mistake-ridden. — Sneagan, Nov 05 '11 at 04:53

C Program terminates after if/else or repeats if I use fputs/fgets

3 Answers3