pset3 recover, only get 49 jpgs and all are wrong by check50, but look correctly

Question

I recently started with CS50 and I struggle a bit with pset3's recover. While the jpgs look correct to me, my code only recovers 49 jpgs total, not the 50 which are expected. Check50 fails on the first jpg as well. Could you take a look at my code and push me in the right direction? I really have no clue what went wrong here.

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

// Make type BYTE
typedef uint8_t BYTE;

//function to check if the first bytes are jpg header bytes
int isJpg(BYTE *buffer);

int main(int argc, char *argv[])
{
    // test for correct amount of arguments
    if (argc != 2)
    {
        fprintf(stderr, "Usage: ./recover file. Please try again.");
        return 1;
    }

    // save file for reading
    FILE *inptr = fopen(argv[1], "r");

    // if inptr points to NULL exit program
    if (!inptr)
    {
        fclose(inptr);
        fprintf(stderr, "File could not be opened.");
        return 2;
    }

    //allocate memory for the buffer
    BYTE *buffer = malloc(512);
    // Set counter to name the jpgs correctly
    int counter = 0;
    // Searching for a jpg
    int searching = 1;
    //make a string for the filenames
    char filename[8];
    // declare global variable, so it can be used in the while loop
    FILE *img = NULL;

    // read and write as long as EOF is not reached
    while (fread(buffer, 1, 512, inptr) == 512)
    {
        if(searching)
        {
            // check if the first 4 Bytes match the jpeg header
            if (isJpg(buffer))
            {
                //use sprintf to correctly name the file
                sprintf(filename, "%03i.jpg", counter);
                //create the file
                img = fopen(filename, "w");
                fwrite(buffer, 1, 512, img);
                searching = 0;
            }
        }
        else
        {
            if(isJpg(buffer))
            {
                fclose(img);
                //use sprintf to correctly name the file
                sprintf(filename, "%03i.jpg", counter);
                //create the file
                img = fopen(filename, "w");
                //up the counter
                counter++;
                fwrite(buffer, 1, 512, img);
            }
            else
            {
                fwrite(buffer, 1, 512, img);
            }
        }
    }
    fwrite(buffer, 1, 512, img);
    fclose(img);
    //free allocated memory
    free(buffer);
}

int isJpg(BYTE *buffer)
{
    if (buffer[0] == 0xff &&
        buffer[1] == 0xd8 &&
        buffer[2] == 0xff &&
        (buffer[3] & 0xf0) == 0xe0)
        return 1;
    else
        return 0;
}

Answer 1

Hmmm.... interesting issue. The code is producing only 49 files when there should be 50. That's really conspicuous. First inclination is to see what's happening with the first file and the last file. So let's look at the creation of the first file. Looking at the code, it seems to be opening and creating the first file ok when running debug. Now, looking at the creation of the last file seems ok too. So there's something else going on. Time to trace the program through and see what's happening.

If you were to trace the program execution, you'd discover something interesting. When the program code steps through and finds the second signature, it opens the second image file with the name 000.jpg!!!!! That's going to delete the first file and create a new 000.jpg file!

The issue becomes quite obvious when you know this and the fix is just as obvious!

This is also a good demonstration of why it's important to learn to use the debuggers and to step through code, and to carefully watch all variables and changes that occur during execution. Also, take advantage of breakpoints so that you can quickly move from one point to another in the code without manually stepping through each line.

I'll leave it to you to think about what's happening. Please post a comment when you figure it out! ;-)

This is also a good demonstration that there's always more than one way to implement an algorithm. This code has a code block to find the first signature and to open the first file, followed by another code block that finds all the following signatures. The thing is, though, that the code blocks are essentially identical. That's a red flag that maybe the code can be made more efficient by having only one code block.

In fact, it can be simplified by having one code block that searches for signatures and opens files for output. The only thing that remains is to figure out how to handle all the junk data that comes before the first signature. The answer is simple. Don't do anything with it. Knowing that all the data before the first signature is trash and all the data that follows is good means that all the code needs to know is if an output file has been opened. If not, don't write the buffer out. If at least 1 file has been opened for output, write the data to the current file.

It's about figuring out when there's a more efficient way to do something. Programming should have at least two phases related to this. Phase 1: create the program that works. Phase 2: analyze the code to see if there's a way to make it run more efficiently.

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)