CODEWITHSUNDEEP
carrayspointersmalloc
Ulysses
Ulysses

Reputation: 17

C Programming: Using malloc, pointers and arrays

I'm a beginner in C programming and was assigned this to complete a few weeks ago but I never figured it out.

Assignment Description

We were given some skeleton code with comments to guide us but I can't understand one of these comments.

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

typedef struct
{
    char firstName[50];
    char lastName[50];
    long id;
    char english, french, maths, philosophy;
} result;

// array of pointers to 'result' structures - assuming that there is 100 or less records in the data file
result *results[100];

// number of records read from the file
int numResults = 0;

// read file and populate the results
// you will need to use malloc to allocate a new 'result' structure for each record read from the file
// the *result pointer returned by malloc will be stored in the next member of the array *results[]
int readFile(char *fileName);

// set all the pointers in *results[] to NULL before reading in the file
void initialiseResults();

// function to print an individual member of the *results[] array
void printResult(result *aResult);

int main()
{
    char fileName[] = "D:\\results.txt";
    int i = 0;

    void initialiseResults();

    if (!readFile(fileName))
    {
        printf("File could not be opened!\n");
        return 0;
    }

    printf("Surname\tFirst Name\tID\tMaths\tEnglish\tFrench\tPhilosophy\n");
    printf("=======\t==========\t==\t=====\t=======\t======\t==========\n");

    while (results[i] != NULL)
    {
        printResult(results[i]);
        i++;
    }

    return 0;
}

void printResult(result *aResult)
{
    printf("%s\t%s\t%d\t%c\t%c\t%c\t%c\n", aResult->lastName, aResult->firstName, aResult->id, aResult->maths, aResult->english, aResult->french, aResult->philosophy);
}

void initialiseResults()
{
    int i;

    for (i = 0; i < 100; i++) {
        *(results + i) = NULL;
    }
}

int readFile(char *fileName)
{
    FILE *fptr = fopen(fileName, "r");

    int i;
    char line[100];

    fgets(line, 100, fptr);

    i = 0;

    while (!feof(fptr)) {
        results[i] = (result*)malloc(sizeof(result));

        fscanf(fptr, "%d\t%s\t%s\t%c\t%c\t%c\t%c", &(*results + i)->id, &(*results + i)->firstName, &(*results + i)->lastName, &(*results + i)->english, &(*results + i)->maths, &(*results + i)->french, &(*results + i)->philosophy);

        numResults++;
        i++;
    }


    return 1;
}

When it states that the *result pointer returned by malloc will be stored in the next member of the array *results[], I'm unsure of what to do here or if possibly my syntax is wrong with the previous arrays and pointers.

Here is the original skeleton code before I made my attempt at it.

#include "stdafx.h"
#include "stdio.h"
#include "string.h"
#include "stdlib.h"

typedef struct 
{
    char firstName[50];
    char lastName[50];
    long id;
    char english, french, maths, philosophy;
} result;

// array of pointers to 'result' structures - assuming that there is 100 or less records in the data file
result *results[100];

// number of records read from the file
int numResults = 0;

// read file and populate the results
// you will need to use malloc to allocate a new 'result' structure for each record read from the file
// the *result pointer returned by malloc will be stored in the next member of the array *results[]
int readFile(char *fileName);

// set all the pointers in *results[] to NULL before reading in the file
void initialiseResults();

// function to print an individual member of the *results[] array
void printResult(result *aResult);

int main()
{
    char fileName[] = "C:\\results.txt";
    int i=0;


    void initialiseResults();

    if (!readFile(fileName)) 
    {
        printf( "File could not be opened !!\n" );
        return 0;
    } 

    while (results[i] != NULL)
    {
        printResult(results[i]);
        i++;
    }

    return 0;
}

void printResult(result *aResult)
{
    // PUT YOUR CODE HERE
}

void initialiseResults()
{
    // PUT YOUR CODE HERE
}

int readFile(char *fileName)
{
    // PUT YOUR CODE HERE

    return 1;
}

Upvotes: 0

Views: 850

Answers (1)

John Bode
John Bode

Reputation: 123596

When it states that the *result pointer returned by malloc will be stored in the next member of the array *results[], I'm unsure of what to do here or if possibly my syntax is wrong with the previous arrays and pointers.

So let's look at your loop where you allocate and read each results[i]:

while (!feof(fptr)) {
    results[i] = (result*)malloc(sizeof(result));

    fscanf(fptr, "%d\t%s\t%s\t%c\t%c\t%c\t%c", &(*results + i)->id, &(*results + i)->firstName, &(*results + i)->lastName, &(*results + i)->english, &(*results + i)->maths, &(*results + i)->french, &(*results + i)->philosophy);

    numResults++;
    i++;
}

The malloc call itself is okay, although it can be more cleanly written as

results[i] = malloc( sizeof *results[i] );

The cast has not been necessary since C891, and the type of the expression *results[i] is result, so sizeof *results[i] gives the same answer as sizeof (result). You should get definitely into the habit of checking the result of any malloc (or calloc or realloc) call, though.

The rest of the loop has a couple of problems. First, your fscanf call is wonky; instead of &(*results + i) (which is incorrect), just use &results[i] (except for firstName and lastName, which I'll explain in a bit). It's a bit easier to read and harder to screw up. Secondly, you don't need the \t in the format string; the %d and %s conversion specifiers will skip over any leading whitespace, and a single blank space between the %c conversion specifiers will do the same thing. Taking it all together, that call can (and should) be written as

fscanf( fptr, "%d %s %s %c %c %c %c", &results[i]->id, results[i]->firstName,
  results[i]->lastName, &results[i]->english, &results[i]->maths,
  &results[i]->french, &results[i]->philosopy );

So why don't we use the & operator for results[i]->firstName and results[i]->lastName? Both the firstName and lastName members are arrays of char; under most circumstances, an expression of type "N-element array of T" will be converted ("decay") to an expression of type "pointer to T", and the value of the expression will be the address of the first element of the array. The %s conversion specifier expects its corresponding argument to be a pointer to char; the expressions results[i]->firstName and results[i]->lastName will have type "pointer to char" because of this conversion rule.

Next, you shouldn't use feof as your loop condition. EOF won't be signaled until after you try to read past the end of the file, so your loop will wind up executing once too often. IOW, you don't know you've read the last record until you try to read one more.

Instead, you should check the status of the last input operation. fscanf will return EOF if it hits end-of-file before reading anything, otherwise it will return the number of items successfully read. You're expecting to read 7 items from the file at a time, so your loop could be rewritten as

while ( fscanf( ... ) == 7 )
{
  // do stuff
}

Except...

What would you fscanf into? You haven't allocated the results[i] element yet, so you can't read into any of its members. There are actually several ways to go from here, each with advantages and disadvantages.

Personally, I'd do the following: read a line from the file as text (as you do in the fgets call, which you never use again), and use sscanf to read elements from that string:

while ( fgets( line, sizeof line, fptr ) ) // fgets returns NULL on EOF or error
{                                          // otherwise it returns line
  results[i] = malloc( sizeof *results[i] );
  if ( results[i] )
  {
    if ( sscanf( line, "%d %s %s %c %c %c %c", &results[i]->id, results[i]->firstName,
      results[i]->lastName, &results[i]->english, &results[i]->maths,
      &results[i]->french, &results[i]->philosopy ) < 7 )
    {
      // we didn't find all the record elements in the line; either the
      // input line is longer than we expect, or it's malformed.  

      fprintf( stderr, "Input line \"%s\" is bad; exiting...\n", line );
      exit( 0 );
    }
    numResults++;
    i++;
  }
  else
  {
    // handle memory allocation failure; for this exercise, you'll just
    // want to print an error message and exit at this point.
    fprintf( stderr, "Memory allocation failure...exiting\n" );
    exit( 0 );
  }
}

There's one critical point that this code doesn't address, though; what if the file contains more than 100 records? Ideally, you'd like to add a second condition to that loop so that you don't try to read more records than you can store in your array. Adding that tweak:

while ( numResults < 100 && fgets( line, sizeof line, fptr ) )
{
  results[i] = malloc( sizeof *results[i] );
  if ( results[i] )
  {
    if ( sscanf( line, "%d %s %s %c %c %c %c", &results[i]->id, results[i]->firstName,
      results[i]->lastName, &results[i]->english, &results[i]->maths,
      &results[i]->french, &results[i]->philosopy ) < 7 )
    {
      // we didn't find all the record elements in the line; either the
      // input line is longer than we expect, or it's malformed.  

      fprintf( stderr, "Input line \"%s\" is bad; exiting...\n", line );
      exit( 0 );
    }
    numResults++;
    i++;
  }
  else
  {
    // handle memory allocation failure; for this exercise, you'll just
    // want to print an error message and exit at this point.
    fprintf( stderr, "Memory allocation failure...exiting\n" );
    exit( 0 );
  }
}

Even more ideally, you'd want to create a symbolic constant like MAX_RECORDS so you're not using naked 100s2 all over the place:

#define MAX_RECORDS 100
...
result *results[MAX_RECORDS];
...
void initialiseResults()
{
  int i;

  for (i = 0; i < MAX_RECORDS; i++) {
    results[i] = NULL;
  }
}
...
while ( numResults < MAX_RECORDS && fgets( line, sizeof line, fptr )
{
  ...
}

It's also not clear to me why you need both i and numResults in readFile. You could get away with just using numResults.

You should also get into the habit of calling free on every object you malloc when you're finished with it (in this case, after you've called printResult), but for the purposes of this assignment that's not critical.

Addendum

It's also worth pointing out that since it's declared at file scope (outside of the body of any function)3, all of the elements of the results array will be implicitly initialized to NULL, so technically the initializeResults function is redundant. However, if you want to tweak this code to handle multiple input files, then you probably would want to re-initialize the array before processing the next file. In that case, you would want to free any previously-allocated elements:

void initialiseResults( void )
{
  for ( int i = 0; i < NUM_RECORDS; i++ )
  {
    free( results[i] ); // free( NULL ) is a no-op, so calling this on any
    results[i] = NULL;  // NULL elements of results[i] won't cause a problem
  }
}

1. It's still necessary for C++, but if you're writing C++ you shouldn't be using malloc anyway.
2. Using "magic" numbers (literals with special meaning beyond their immediate value) makes code harder to maintain; if you change the size of the array from 100 to 200, then you have to chase down every instance of 100 in your code, decide if it's being used to indicate the maximum array size, and change it if it is. Using a symbolic constant such as the MAX_RECORDS macro means you only have to make that change in one place.
3. Ideally, you should not use such "global" variables in your code; they wind up creating a lot of maintenance headaches and hinder re-use (you wouldn't be able to use any of those functions in a different program unless it also declared a global "results" array). It would be better to declare results in main and pass it as an argument to each of the initialiseResults, readFile, and printResult functions, but I understand you didn't make that decision. Having said all of that, there are times when globals are the right answer, especially in the embedded world where stack space is extremely limited.

Upvotes: 1

Related Questions