0

I need some guidance into what I'm doing wrong. Apparently you can't access an individual byte if you store it as a block of 512 using fread() from what check50 is telling me, I think thats one of the many mistakes I'm doing. Anyways here's my code:

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


bool check(char buffer[]);


int main(int argc, char* argv[])
{

    // open memory card file
    FILE *fp_input = fopen("card.raw", "r");
    if (fp_input == NULL)
    {
        printf("Couldn't open %s.", "card.raw");
        return 1;
    }


    char buffer[512];                   //char data type = 1 byte
    char title[8];                     // ex: title = "002.jpg"
    int counter = 0;


    while(feof(fp_input) == 0)    //feof returns nonzero if end-of-file indicator is set
    {  

        fread(buffer, 512, 1, fp_input);

        if (check(buffer) == true)   //if its a start of a new jpg
        {
            counter++;
            if (counter == 1)
            {
                sprintf(title, "%03d.jpg", counter);     
                FILE *fp_output = fopen(title, "a");     //create new file named 001.jpg
            }

            else //if counter is not 1  (ex: counter = 2)
            {
                fclose(fp_output);   //close last one (close 001.jpg)
                sprintf(title, "%03d.jpg", counter);    //002.jpg
                FILE *fp_output = fopen(title, "a");    //open 002.jpg
                if (fp_output == NULL)
                {
                    fprintf(stderr, "Couldn't open %s.", title);
                    return 1;
                }

            }
        }       

        if (fp_output != NULL)  //if fp_out is already opened
            fwrite(buffer, 512, 1, fp_output);           //keep writing in it

    } 


    fclose(fp_input);
    fclose(fp_output);
    return 0;
}

bool check(char buffer[])   //checks if first 4 bytes are start of jpeg
{
    if(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff    //if jpg is found
                  && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
                  return true;              //returns true if start of jpeg
    else
        return false;                       //returns false if not start of jpeg
}

Please feel free to criticize it

UPDATE:

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

typedef uint8_t  BYTE;    //or unsigned char

bool check(BYTE buffer[]);

int main(void)
{

    // open memory card file
    FILE *fp_input = fopen("card.raw", "r");
    if (fp_input == NULL)
    {
        printf("Couldn't open %s.", "card.raw");
        return 1;
    }


    BYTE buffer[512];                   
    char title[8];                     // ex: title = "002.jpg"
    int counter = -1;
    FILE *fp_output = NULL;


    while(feof(fp_input) == 0)    //feof returns nonzero if end-of-file indicator is set
    {  

        fread(buffer, 512, 1, fp_input);

        if (check(buffer) == true)   //if its a start of a new jpg
        {
            counter++;
            if (counter == 0)
            {
                sprintf(title, "%03d.jpg", counter);     
                fp_output = fopen(title, "a");     //create new file named 000.jpg
            }

            else //if counter is not 0  (ex: counter = 1)
            {
                fclose(fp_output);   //close last one (close 000.jpg)
                sprintf(title, "%03d.jpg", counter);    //001.jpg
                fp_output = fopen(title, "a");    //open 001.jpg
                if (fp_output == NULL)
                {
                    fprintf(stderr, "Couldn't open %s.", title);
                    return 1;
                }

            }
        }       

        if (fp_output != NULL)  //if fp_out is already opened
            fwrite(buffer, 512, 1, fp_output);           //keep writing in it

    } 

    fclose(fp_input);
    fclose(fp_output);
    return 0;
}

bool check(BYTE buffer[])   //checks if first 4 bytes are start of jpeg
{
    if(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff    //if jpg is found
                  && (buffer[3] >= 0xe0 && buffer[3] <= 0xef) )
                  return true;              //returns true if start of jpeg
    else
        return false;                       //returns false if not start of jpeg
}

Runnning check50:

~/workspace/pset4/jpg/ $ check50 2015.fall.pset4.recover recover.c
:) recover.c exists
:) recover.c compiles
:) recovers 000.jpg correctly
:) recovers middle files correctly
:( recovers last file correctly
1

The problems are pretty straightforward. buffer[] is being declared as an array of chars. The problem with that is that a char behaves like a one byte signed integer. That introduces issues related to twos compliment notation and processing. You should consider an unsigned data type. (Since you need to learn about data types, I'll let you research it. Try to google "data types in c")

Next, you've declared your fp_output file pointer inside the code block for an if statement. By now, you should know that the scope of that var will end with the curly brace that follows, so it doesn't exist later when you try to use it.

I get the feeling from fp.close(fp_output); that you may have some object oriented programming experience. fp.close is not valid in c.

There may well be other issues and you might introduce new ones, but this should get you going.

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

6
  • thanks, I'll try to fix it
    – tadm123
    Dec 20 '16 at 2:35
  • would declaring the fp_output variable as extern variables fix this problem? extern FILE *fp_output = fopen(title, "a");
    – tadm123
    Dec 20 '16 at 2:47
  • The simple thing to do would be to declare it earlier in the program, like at the beginning of main, or in the same block of code where you declare buffer, title and counter. There's no call or need here to declare an extern or global var. You can always (and should) initialize it to NULL and then set it later.
    – Cliff B
    Dec 20 '16 at 3:17
  • can you give me a last piece of advise? I updated the code in the OP. I fixed all the mistakes that you pointed out before and now and it's running properly except for some mistakes. Thanks again.
    – tadm123
    Dec 20 '16 at 4:44
  • It's a common problem. The code reads in a block, processes it, writes out to the output file, and then comes back to the beginning to check for EOF. This means that an extra 512 bytes are being written to the last file. Gee, if only it were possible to incorporate the fread into the EOF check in the while loop setup statement. ;-)
    – Cliff B
    Dec 20 '16 at 5:01

You must log in to answer this question.

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