Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code?

What's wrong with this code?

  • Comments 29

This one comes courtesy of a co-worker who once upon a time was the lead developer for the Manx ( compiler.

He ran into the following problem that was reported by the customer.  They tried and tried to get the following code to compile and it didn't seem to work:


#include <stdio.h>
float ReadFileAndCalculateAverage(FILE *InputFile, int Values[], int LengthOfValues, int *NumberOfValuesRead)
   float average;
   int i;

   // Read the Values array from InputFile.

   <Code eliminated for brevity>

   // Now calculate the average.

   for (i = 1 ; i <= *NumberOfValuesRead ; i += 1)
      average += Values[i];

   average = average/*NumberOfValuesRead;

int main(int argc, char *argv[])
    int Values[20];
    float average;
    int numberOfValues;
    FILE *inputFile;

    inputFile = fopen(“MyFile.Txt”);
    if (inputFile != NULL)
        average = ReadFileAndCalculateAverage(inputFile, Values, sizeof(j) / sizeof(j[0]), &numberOfValues);

 Now imagine this problem in a 10,000 line source file :) Have fun trying to track it down.

Edit: Removed bogus <p>'s.  Thank you Newsgator's .Text plugin...

  • /* is a comment, not division followed by pointer dereference :)
  • there is no j.. what is j?

    and /* is not what you think it is there
  • Grr - left the 'j' from a previous version. :)
  • I thought any sane developer uses an editor/IDE with syntax highlighting. Any software capable of this, be it Visual Studio, VIM or even EMACS would show that code as if it was a comment. Pretty trivial if you ask me
  • To my knowledge, vi doesn't, neither do many versions of emacs (some do, some don't).

    Other editors optionally enable syntax coloring.

  • you're probably thinking of VIM, which, when installed, creates a symbolic link vi that actually starts vim with syntax highlighting
  • float average in ReadFileAndCalculateAverage is uninitialized
  • In main, j is undeclared. It seems that j should be Values. Ignoring the eliminated code, it seems that average may be uninitialized. Maybe that was eliminated for brevity. The for loop starts at 1. It should start at 0 and use < rather than <=. After the loop it might be sensible to test for there being 0 values read...

    /* is already pointed out... That is the tricky part, but the other errors are pretty serious.

  • the lack of a return statement

    (in addition to the /*)
  • also fopen requires a 2nd parameter specifying the access permitted to "MyFile.Txt"
  • Vim does syntax highlighting. Only a masochist would use vi.
  • Hey, whaddya know, open source works! :)
  • fclose should be inside the if block. I'm not sure what the effect of trying to fclose a NULL pointer is, but I'm not going to take the risk.

  • What about the array? You reserve space for a value[20] array but you don't limit the number of values you read. So when you compute the average you might try to access value[x] where x > 20?
  • Easy. The filename parameter to fopen() uses typographical quotation marks instead of straight quotes.

    That is:
    inputFile = fopen(“MyFile.Txt”);

    should be:

    inputFile = fopen("MyFile.Txt");

    This is what you get when newspaper editors write code ;-)
Page 1 of 2 (29 items) 12