D.B. D.B. - 3 months ago 14
C Question

Improving C code to remove copy-paste

P.S. Some people had issues understanding the question 'how to improve my code'. I couldn't have made it clearer, really. I solved the issue in the meantime.

I'm working on an assignment to recover photos from a cd card file (.raw) by first finding them by their 'magic numbers'. It's a part of a CS50 class if anybody knows what it is.

Anyway, it seems to work ok-ish. I can recover all photos, but some of them of a worse quality, then others (a lot of white pixels) - not sure if it's intended.

In the code depending on conditions I open and close jpg files to write info into them. I can't figure out a way to avoid copy-paste to fopen, fwrite and fclose the file. I'd like to improve this part. Here's the code:

typedef uint8_t BYTE;

int main(void)
{
// open input file
FILE* card_file = fopen("card.raw", "r");
if (card_file == NULL)
{
printf("Could not open card.raw\n");
return 1;
}

// declare output file
FILE* image;

// array to store values we read for each 512 bytes
BYTE buffer[512];

// number of jpg files found
int jpgs = 0;

// variable used to create new files
char title[10];

// execute until the end of file is reached
while(fread(&buffer, sizeof(BYTE), 512, card_file) > 0)
{
// find a new jpg file by its magic numbers
if((buffer[0] == 0xff) && (buffer[1] == 0xd8) && (buffer[2] == 0xff))
{
//create a new file if it's found
sprintf(title, "%03d.jpg", jpgs);
jpgs++;

//open the file and check if file is opened correctly
image = fopen(title, "a");

if (image == NULL)
{
fprintf(stderr, "Could not create %03d.jpg\n", jpgs);
return 2;
}

// write into the file and close it
fwrite(&buffer, sizeof(BYTE), 512, image);
fclose(image);
}
else
{
//check if a jpg file was already found before
if(jpgs > 0)
{
//open the file and check if file is opened correctly
image = fopen(title, "a");

if (image == NULL)
{
fprintf(stderr, "Could not create %03d.jpg\n", jpgs);
return 2;
}

// write into the file and close it
fwrite(&buffer, sizeof(BYTE), 512, image);
fclose(image);
}
}
}

//close the input file
fclose(card_file);

}


I tried to create a separate function, but couldn't figure out how to handle pointer to integer values I need to pass to it.

Any help would be appreciated.

Answer

The only things that need to be passed by reference are the title and buffer, you need to return a boolean that we can test to allow the return 2; on the caller.

int appendBuffer(char *title, BYTE *buffer)
{
    //open the file and check if file is opened correctly
    FILE* image = fopen(title, "a");

    if (image == NULL)
    {
        fprintf(stderr, "Could not open %s\n", title); //note reuse of title now
        return 0; //it didn't work
    }

    // write into the file and close it
    fwrite(buffer, sizeof(BYTE), 512, image);
    fclose(image);
    return 1; //it worked
}

Usage:

// find a new jpg file by its magic numbers
if((buffer[0] == 0xff) && (buffer[1] == 0xd8) && (buffer[2] == 0xff))
{
    //create a new file if it's found
    sprintf(title, "%03d.jpg", jpgs);
    jpgs++;

    if (!appendBuffer(title, buffer)) return 2;
}
else 
{
    //check if a jpg file was already found before
    if(jpgs > 0)
    {
       if (!appendBuffer(title, buffer)) return 2;
    }
}

Another idea for comparing many sequential bytes, use memcmp rather than && chains:

const char magic[] = {0xff, 0xd8, 0xff};

// find a new jpg file by its magic numbers
if (memcmp(buffer, magic, 3) == 0)