why do i have to check the file to be null in cs50 recover

Question

At the end of the code why do i have to check if img is not null before writing why can we not write it like in the second snippet? why do we have to check it?

FILE *input_file = fopen(argv[1], "r");
//if check Input_file pointer fail to open then REturn error code "Could not open file"
if (input_file == NULL)
{
    printf("Could not open file");
    return 2;
}

//declare a variable to unsigned char to store 512 chunks array
unsigned char buffer[512];

//for the purpose of counting of image later in the loop
int count_image = 0;

//An uninitialize file pointer to use to output data gotten from input file
FILE *output_file = NULL;

char *filename = malloc(8 * sizeof(char));
//char filename[8];

/*Read 512 bytes from input_file and store on the buffer*/
while (fread(buffer, sizeof(char), 512, input_file))
{
    //check if bytes is start of a JPEG
    if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
    {
        //write jpeg into file name in form 001.jpg, 002.jpg and so on
        sprintf(filename, "%03i.jpg", count_image);

        //open Out_file for writing
        output_file = fopen(filename, "w");

        //fwrite(buffer, sizeof(buffer), 1, output_file);
        //count number of image found
        count_image++;
    }
    //Check if output have been used for valid input
    if (output_file != NULL)
    {
        fwrite(buffer, sizeof(char), 512, output_file);
    }

}
free(filename);
fclose(output_file);
fclose(input_file);

return 0;
}

cant i write directly like in the code below as the the img pointer will not be null if the if condition is met

    if(buffer[0]==0xff && buffer[1]==0xd8 && buffer[2]==0xff && (buffer[3] & 0xf0) == 0xe0)
{

    sprintf(filename, "%03i.jpg", counter);
    img = fopen(filename, "w");
    fwrite(buffer,sizeof(uint8_t),512,img);
    counter++;
}

so why do we have to check seperately if it is null or not

You are _leaking_ file stream descriptors. You must close the old output stream before opening the new one. So, _before_ the `fopen`, you need: `if (output_file != NULL) fclose(output_file);` — Craig Estey, Jul 23 '22 at 22:21

score 2 · Accepted Answer · answered Jul 23 '22 at 16:33

2

fopen(filename, "w") can return NULL, if for example you don't have permission to create the file. So you need to check that the file pointer isn't NULL anyway. It doesn't matter whether it's nested inside the other if or not.

Also, you should call fclose(img); after you're done with it to close the file, which frees up resources and also makes sure the contents are fully flushed to the file. The first code snippet you've shown only closes the last file created since it's outside the while loop.

answered Jul 23 '22 at 16:33

Kevin

6,993
1
15
24

what i was trying to ask was that in the first snippet at the end before writing to the new file it checks if the output_file != NULL. Why cant it be done like in the second snippet all in a single if as if the if condition (buffer[0]==0xff && buffer[1]==0xd8 && buffer[2]==0xff && (buffer[3] & 0xf0) == 0xe0) is met the file will be open otherwise it will not open so why check in the second if. – ptk03 Jul 23 '22 at 16:45
@ptk03: What makes you so sure that the file will open successfully if the preceding `if` statement is true? That seems like a dangerous assumption. There are many possible reasons for opening a file to fail. For example, it is possible that creating a new file will fail due to running out of disk space. In that case, if you do not check whether `fopen` returns `NULL`, your program will probably crash. You should always check the return value of `fopen` before using the returned `FILE *`. – Andreas Wenzel Jul 23 '22 at 17:04
@ptk03 As I said, the `fopen` call can fail. So even if you get into that `if`, the file is not necessarily created. So you still need to check for `NULL`. – Kevin Jul 23 '22 at 17:15
ok got what you are trying to say, but just for my clarity I want to ask lets **assume** the file opens all the time without any issue in this **hypothetical** case i dont have to check if (output_file != NULL) { fwrite(buffer, sizeof(char), 512, output_file); } i can simply use fwrite in the preceeding if. – ptk03 Jul 23 '22 at 17:15
@ptk03: Yes, if you never check the return value of `fopen`, then you will not have any trouble, as long as all calls to `fopen` succeed. However, if your assumption that `fopen` succeeds is ever false, then your program will likely crash. Therefore, it is usually better to check the return value of `fopen` and to print a proper error message, instead of letting your program crash. – Andreas Wenzel Jul 23 '22 at 17:17
@ptk03 Sure, if the function is guaranteed to not return `NULL` you can do what you suggested in your second code snippet. But it isn't, so you can't. – Kevin Jul 23 '22 at 17:18
@ptk03: Always checking the return value of `fopen` will also make debugging easier. Otherwise, if your program crashes, you will not know whether it crashed due to `fopen` failing or for some other reason. – Andreas Wenzel Jul 23 '22 at 19:00

why do i have to check the file to be null in cs50 recover

1 Answers1