Let's get better in C - Exercise #1 - What's the problem with the following code?

+3 votes
asked Sep 3 by Peter Minarik (7,830 points)
edited Sep 4 by Peter Minarik

Instructions

This exercise is for anyone who would like to get better in C/C++ coding.

I've provided some sample code, that has some problem with it. (At least one, that I wanted to focus on in this exercise, but accidentally I could have made multiple mistakes. XD)

Let's see who finds the problem.

Please, tell me

  1. what part of the code has a problem
  2. why it is a problem
  3. how would you fix it

The Code (with a problem):

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

#ifdef __cplusplus
#define filename "main.cpp"
#else
#define filename "main.c"
#endif

// Summary:         Opens fileName and reads maximum maxByteCount number of bytes from the first line of it and prints it on stdout.
// fileName:        the path to the file to open
// maxByteCount:    the maximum number of bytes to read
void PrintFirstLine(const char * fileName, int maxByteCount)
{
    FILE * file = fopen(fileName, "r");
    char buffer[maxByteCount + 1]; // +1 for reserving space for the terminating zero

    if (file)
    {
        fgets(buffer, sizeof(buffer), file);
        printf("First line read: %s\n", buffer);
    }
    else
    {
        printf("Nothing read.\n");
    }

    fclose(file);
}

int main()
{
    PrintFirstLine(filename, 100);
    return 0;
}

2 Answers

+1 vote
answered Sep 5 by xDELLx (2,950 points)
selected Sep 7 by Peter Minarik
 
Best answer

A good working example how subtle bugs can crash you program.

The BUG


fclose(file);

Above statement will always close a valid file handle which was returned after a fopen() call was made.By chance the fopen() failed , we shouldnt be attempting to close the file handle.Attempting to do so will crash the program.

The Solution, before closing a file check its validity:

    if (file)  {
       fclose(file);
    }

If the aboce program was implemented mainly in C++, a smart resource handler could be used , which is based on the RAII concept[Resource Acquisation/allocation Is Initialisation] 

The file (resource) could be wrapped in a object ( when object goes out of scope, it calls its destructor ) and the clean up of object can be automated.

Below is an example



/******************************************************************************

Welcome to GDB Online.
GDB online is an online compiler and debugger tool for C, C++, Python, Java, PHP, Ruby, Perl,
C#, VB, Swift, Pascal, Fortran, Haskell, Objective-C, Assembly, HTML, CSS, JS, SQLite, Prolog.
Code, Compile, Run and Debug online from anywhere in world.

*******************************************************************************/
#include <iostream>
#define CHATTY 1    //CHATTY 0 //to Disable chatty message when functions called

using namespace std;

class myFileobject
{
  private:FILE * __file = nullptr;
public:
  myFileobject (const char *fileName)
  {
    __file = fopen (fileName, "r");
#if CHATTY
    cout << "myFileobject(const char* fileName) " << fileName << "\n";
#endif
  }
myFileobject (FILE * fp = nullptr):__file (fp)
  {
#if CHATTY
    cout << "myFileobject(FILE* fp=nullptr) \n";
#endif
  };
  ~myFileobject ()
  {

#if CHATTY
    cout << "~myFileobject() __file \n";
#endif
    if (__file)
      fclose (__file);
  }
  operator  bool ()
  {
#if CHATTY
    cout << "operator bool\n";
#endif
    return (bool) __file;
  }
  operator  FILE *()
  {
#if CHATTY
    cout << "operator FILE*\n";
#endif
    return __file;
  }

};

#ifdef __cplusplus
#define filename "main.1cpp"
#else
#define filename "main.c"
#endif

// Summary:         Opens fileName and reads maximum maxByteCount number of bytes from the first line of it and prints it on stdout.
// fileName:        the path to the file to open
// maxByteCount:    the maximum number of bytes to read
void
PrintFirstLine (const char *fileName, int maxByteCount)
{
  //FILE * file = fopen(fileName, "r");
  myFileobject file (fileName);
  char buffer[maxByteCount + 1];    // +1 for reserving space for the terminating zero

  if (file)
    {
      fgets (buffer, sizeof (buffer), file);
      printf ("First line read: %s\n", buffer);
    }
  else
    {
      printf ("Nothing read.\n");
    }

  //fclose(file);
}

int
main ()
{
  PrintFirstLine (filename, 100);
  return 0;
}


commented Sep 7 by Peter Minarik (7,830 points)
Exactly.

The code works fine, until you run into an unexpected case (file cannot be open), when the code crashes due to fopen() trying to close an invalid file handle.

An excellent answer.

Well done! ;)
0 votes
answered Sep 7 by worthwhile (140 points)
The answer is simple, as shown below without any extravagance build up. The program would work normal if the file exists, and the file would close by the system by default.  But if the file did not exist, thats where you would get the program crash.   I may not be syntactically correct and perfect, but the idea is there. But the idea is simple in approach.    Try this idea on for size.

         worthwhile .

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

#ifdef __cplusplus
#define filename "main.xcpp"
#else
#define filename "main.c"
#endif

// Summary:         Opens fileName and reads maximum maxByteCount number of bytes from the first line of it and prints it on stdout.
// fileName:        the path to the file to open
// maxByteCount:    the maximum number of bytes to read
void PrintFirstLine(const char * fileName, int maxByteCount)
{

  char str[20]; /// <- this is your maxByteCount size of 20
  
  fgets(str, 20, stdin); // read from stdin
  puts(str); // print read content out to stdout
  
  // open the file
  FILE *f = fopen("file.txt" , "r");
  
  // if there was an error
  if(f == NULL){
    perror("Error opening file"); // print error
    return(-1);
  }
  // if there was no error
  else{
    fgets(str, 20, f); // read from file
    if(f ==feof(f)) {
        perror("Early File EOF has occurred. "); // file eof.
        return(-1);
    }
    while(!feof(f)) { /// do while not eof(f)
       puts(str); // print out text line.
       fgets(str, 20, f); // read from file
    }
    fclose(f); // close file
  }
  return(0);
}
int main()
{
    PrintFirstLine(filename, 100);
    return 0;
}
Welcome to OnlineGDB Q&A, where you can ask questions related to programming and OnlineGDB IDE and and receive answers from other members of the community.
...