D.B. D.B. - 1 year ago 46
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 Source

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)