0

I am iterating over 1 BYTE at a time, reading it from file to allocated memory, then checking if it's the start of a new jpg, or if it's the end of the file, and if not, writing it to the currently open jpg file (img). This code works, however I see that I check if it's a new jpg header, or if it's the end of the file, then write, then check the same thing over again before starting the loop again. Since each loop is checking these 2 conditions twice, I reckon it is going to use unnecessary computing power making the code slower. Thanks for any suggestions! 

do
{    
     fread(p, sizeof(BYTE), 512, file);

     if (!(p[0] == 0xff && p[1] == 0xd8 && p[2] == 0xff && (p[3] & 0xf0) == 0xe0) && (!feof(file)))
     {
          fwrite(p, sizeof(BYTE), 512, img);
     }
}
while (!(p[0] == 0xff && p[1] == 0xd8 && p[2] == 0xff && (p[3] & 0xf0) == 0xe0) && (!feof(file)));
Ryan Eom
  • 337
  • 3
  • 14
  • 3
    Instead of (**wrongly**) checking `feof()`, check the return value of `fread()` – pmg May 20 '20 at 08:27
  • when the pointer reaches end of file, it evaluates to true. I know because I used to have it only for the while boolean and not the if boolean, and it printed the last BYTE to the jpg, making the output of the 50th, last image file different from the expected image by 1 BYTE. – Ryan Eom May 20 '20 at 08:30
  • Ok, I can change the condition to check fread(). Thank you. Now how do I improve the loop so that it doesn't check the same condition twice in one iteration? – Ryan Eom May 20 '20 at 08:40
  • *"when the pointer reaches end of file, [`feof()`] evaluates to true."* Sorry, no. `feof()` does not report about the current (or previous or future) position of the file-pointer. **`feof()` tells you if the most recent error for the file was due to the file having no more data**... checking `feof()` in the absence of a read error is ..well.. strange (to put it mildly) – pmg May 20 '20 at 08:53
  • What does this description of feof() mean in the context of what we're discussing? "The function feof() tests the end-of-file indicator for the stream pointed to by stream, returning nonzero if it is set. The end-of-file indicator can be cleared only by the function clearerr()." Thank you. – Ryan Eom May 20 '20 at 09:17

1 Answers1

1

Well, as people have stated in the comments, you should not use while(!feof(file)) and it is explained here

But to focus on other stuff. You basically have this:

do {
    // Read
    if(<condition>) {
        // Write
    }
} while(<condition>);

Provided that evaluating the condition does not contain side effects and that the "// Write" cannot influence the condition, this is equivalent to:

do {
    // Read
    if(<condition>) {
        // Write
    } else {
        break;
    }
} while(1);

or (my favourite) this:

do {
    // Read
    if(! <condition>) break;
    // Write
} while(1);

But as a general advice when it comes to performance, you should profile your code to find out if the performance bottleneck really is where you think it is. To me, the duplication of code seems like a much more important issue than the performance. Important enough to rewrite it.

klutt
  • 30,332
  • 17
  • 55
  • 95
  • Thanks both of you. Stupid question, (I only started coding 10 days ago), how do I check the fread return value? (x < 512) but what do I write as x? – Ryan Eom May 20 '20 at 08:57
  • `int x = fread(...); if (x < 512) /*...*/;` or directly `if (fread(...) < 512) /*...*/;` – pmg May 20 '20 at 08:59
  • Ugh the image files did not come out right. I think while the program was checking the boolean, it ran fread, moving the pointer by one BYTE.. – Ryan Eom May 20 '20 at 09:08
  • I fixed it by doing the int x = fread(...) method. Thank you! – Ryan Eom May 20 '20 at 09:39
  • @RyanEom This goes for ALL functions when you want to check the return value: Read the documentation and find out what it does return. – klutt May 20 '20 at 10:39