0

here is my code

 while (fread(buffer, 512, 1, inptr) == 1)
    {
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
        {
            char filename[8];
            start:
            sprintf(filename,"%03i.jpg", counter);
            FILE *outptr = fopen(filename,"w");
            fwrite(buffer, 512, 1,outptr);

            //check next byte
             while (fread(buffer, 512, 1, inptr) == 1)
             {
                  if (buffer[0] != 0xff && buffer[1] != 0xd8 && buffer[2] != 0xff && (buffer[3] & 0xf0) != 0xe0)
                  {
                      fwrite(buffer, 512, 1,outptr);
                      printf("%i",counter);
                  }

                  //if next file starts
                  if(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
                  {
                      counter++;
                      printf("%i",counter);
                      fclose(outptr);
                      goto start;
                  }
             }
             if (counter > 1)
             {
                 fclose(outptr);

             }
        }


    }
    fclose(inptr);
}

images are not produced properly

3

EEK! A GOTO statement!!!! We'll come back to that.

The second IF statement is bad. You can't simply replace every == with != to find the opposite condition of a complex logical test. If any of the 4 bytes were to match, it would cause the test to fail and the data block to not be written out. Think carefully about what would happen if the first byte of the data block were 0xFF.

There's a semester of digital logic that explains this, but it simply doesn't work. Instead, the easy way to do it is to take the original complex logic statement, surround it with a pair of parentheses, and put a ! in front of it. For example, to get the negative of a==b && c==d, do this: !( a==b && c==d ).

The code is finding false signatures, causing the program to not write out some of the image data, thus corrupting the image files.

Now, let's look at some programming issues.

First, the code uses two separate if statements inside the inner while loop. Because of the logic test error above, it's possible that both if statements can be false at the same time. That will cause a data block to be discarded without being processed.

In this case, you want the code to absolutely do one thing or the other. This calls for an if/else setup. Logically, a data block will have a signature or it won't. Therefore, it must be processed one way or another. An if/else statement makes it impossible for both conditions or neither condition to be processed. It must process one way or the other. (It's also more efficient to do if/else than to have two different if statements because the code is only processing one logical test.)

Next, there are 3 if statements - two are identical and one is nearly the same. There are also multiple while/fread statements. Anytime you have the same code or very nearly identical code, it should always be a red flag that code is being duplicated needlessly. The code may work, but probably isn't the most efficient way to write it. I'm not saying that it should never be done, but it probably can be written more efficiently without duplicating code. (The exceptions are usually related to optimizing code for very specific efficiencies at execution time. But that's an entirely too complex topic at this point.)

In this case, its possible to write the code with a single while/fread statement and a single if statement to check for signatures. I'll leave it to you to think about how you could rewrite the code without the duplicate statements.

Finally, there's the GOTO statement. A GOTO statement should always be a really big red flag that something is really wrong. GOTO statements should be avoided like the plague. If you're including one, then a rethink is definitely in order. I'm not saying they should never be used, but there needs to be a really, really good reason to use one. It's universally considered to be a bad practice to use a GOTO statement. It's a very ugly way to jump from one point in code to another and, more often than not, leads to logic and flow errors in the code!

So, you'd benefit a lot from a rethink of the code to see if you can make it cleaner and more efficient! Happy coding!

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

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .