C fgets question

Question

struct DVDInfo  *ReadStruct( void ) {
    struct DVDInfo  *infoPtr;
    int             num;
    char            line[ kMaxLineLength ];
    char            *result;

    infoPtr = malloc( sizeof( struct DVDInfo ) );

    if ( NULL == infoPtr ) {
        printf( "Out of memory!!!  Goodbye!\n" );
        exit( 0 );
    }

    printf( "Enter DVD Title:  " );
    result = fgets( line, kMaxLineLength, stdin );
    line[ strlen( line ) - 1 ] = '\0';
    infoPtr->title = MallocAndCopy( line );

    printf( "Enter DVD comment:  " );
    result = fgets( line, kMaxLineLength, stdin );
    line[ strlen( line ) - 1 ] = '\0';
    infoPtr->comment = MallocAndCopy( line );

    do {
        printf( "Enter DVD Rating (1-10):  " );
        scanf( "%d", &num );
        Flush();
    }
    while ( ( num < 1 ) || ( num > 10 ) );

    infoPtr->rating = num;

    printf( "\n----------\n" );

    return( infoPtr );
}

What is the purpose of even having the variable "result" above? Nothing is done with it. The pointer returned from fgets is stored into it, but that is it, it has no purpose.

Answer 1

You should test that result for NULL, to check for an EOF condition or an error, instead of just ignoring it. Also, by not checking result, you are doing an strlen on line, which could have uninitialized data, because fgets failed. Really, you should have, after the fgets:

if (!result)
{
  free(infoPtr); // To not leak the object allocated at the start
  return NULL; // Function failed
}

You might still have leaks, if the first fgets succeeds and the second fails, because there are additional allocation to pointer members of the structure. Unfortunately, because the struct was not initialized to zero, you can't check those pointers for NULL. So, perhaps using calloc instead of malloc or at least initializing all structure pointer members to NULL, would have been a better idea.

Answer 2

It seems as though someone started to implement error checking, but botched it in the end. The return value should be compared with NULL, with an error reported if equal.

Answer 3

Most likely, the compiler threw a warning about a function return value that was ignored. The programmer didn't care about the return value of fgets and simply added in the result = to make the compiler quit nagging about it. The correct solution would be to check the return value to make sure the function completed successfully.