Mathstudent Mathstudent - 3 months ago 10
C Question

Looping through end of data file

I've been working on a problem set for the Harvard CS50 class and we have been tasked with recovering jpegs from a memory card. The card stores the jpgs sequentially. In writing my program I decided on using a while loop to keep looping through until EOF, but using the debugger included with the course I am finding that my loop never initiates. I have included my code below and I am really hoping somebody can help me understand where I went wrong in my loop.

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

int main(int argc, char* argv[])
{
// Ensure proper Usage
if (argc != 1)
{
printf("This file takes no input commands!\n");
return 1;
}

// open up card data file
FILE* dataFile = fopen("card.raw", "r");
if (dataFile == NULL)
{
char* invalidFile = "card.raw";
printf("Could not open %s.\n", invalidFile);
return 2;
}

// Create variable to keep track of num of output files written
int numFiles = 0;

// Create buffer
int* buffer = (int*)malloc(sizeof(int*) * 512);

// Create new file conditions
bool a = buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff;
bool b = buffer[3] == 0xe0 || buffer[3] == 0xe1|| buffer[3] == 0xe2 ||
buffer[3] == 0xe3 || buffer[3] == 0xe4 || buffer[3] == 0xe5 ||
buffer[3] == 0xe6 || buffer[3] == 0xe7 || buffer[3] == 0xe8 ||
buffer[3] == 0xe9 || buffer[3] == 0xea || buffer[3] == 0xeb ||
buffer[3] == 0xec || buffer[3] == 0xed || buffer[3] == 0xee ||
buffer[3] == 0xef;

// Loop through until all files found
while(fread(&buffer, 512, 1, dataFile) == 1)
{
if(a && b)
{
// Create temporary storage
char title[999];
// print new file name
sprintf(title, "%d.jpg", numFiles);
// open new file
FILE* img = fopen(&title[numFiles], "a");
numFiles = numFiles + 1;
fwrite(&buffer, sizeof(buffer), 1, img);
free(buffer);
}
else
{
if(numFiles > 0)
{

}
}
}
}

Answer

Okay you might have more than one misunderstanding.

First of all take a look at the man page of fread:

On success, fread() and fwrite() return the number of items read or written.

So if you request 512 bytes you should expect it to return 512. If it returns less, you know: Either you reached the end of your file or something went wrong.

Also if you assign:

bool a = something;
bool b = somethingElse;

And then:

while(somethingEntirelyElse) {
    // Never update the value of a or b
    if(a && b) { ... }
    // Neither here ...
}

Then you could call that check redundant couldn't you?

That is unless a and b actually are volatile. But don't go there!

In your case the assignment of a and b most likely belongs inside the while loop.

Then you alloc your buffer once outside the loop, but free it each time you actually write it down. On the next iteration you will get a segmentation fault.

Have you considered what happens if any of the image files is any other size than 512 bytes, and the images are not aligned properly?

A better way would be to save the position of the start of one file and once you encounter the EOF (which you aren't really looking for...) you would know the exact size of the file, and how big your buffer would have to be.


In conclusion I would agree with some of the commenters, that you really should have a look at some earlier assignments and see whether you can learn something from them.


Analogy time:

Let's assume you have a book, and you want to copy each page that has an image on it. You tell yourself: Aw I just follow a simple algorithm, so I can think about something else.

Your algorithm is:

  • Have a photocopy machine

  • Have a notebook on which you note whether a page contains an image or not.

  • Take a look at the (as of yet closed book) and see if there is an image. -> Write down result on notebook

  • Start turning the pages as long as the next page has exactly one letter on it. (No more no less) -> you stop immediately in most cases

  • If the notebook says: There is an image, copy this page (note that the notebook will say whatever you wrote on it the first time)

  • If you just copied an image, throw away your copying machine (and don't bother buying a new one for the next image)