Display rgb values of ppm image and conflict type error in C

Question

I'm fairly new to C and have been tasked with creating a program that reads an image and displays that image. I've created (what I believe to be) a function to read in the files format/comments etc, and have attempted to create a function to display each pixels rgb values.

I cannot test it because I have an error in my program - 'conflicting types for getPPM'. Could anyone explain why I'm getting this error? and any possible fix to this would be great.

If anyone could suggest a way to be able to store the comments (maybe in array?) and print them later on that would also be really helpful! Thanks.

Also feel free to scrutinize the program as a whole and give me feedback, I'm not entirely sure if it will actually do anything once the above error is fixed. Thanks very much!

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


#define MAX_HEIGHT 600
#define MAX_WIDTH 400

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

    // get image file
    FILE *file;
    file = fopen("aab(1).ppm", "r");

    //if there is no file then return an error
    if(file == NULL){
        fprintf(stderr, "File is not valid");
        return 0;
    }
    else {
        struct PPM *newPPM = getPPM(file);
        return 0;
    }
}

struct PPM {
    char format[2]; //PPM format code
    int height, width; //image pixel height and width
    int max; //max rgb colour value

} PPM;

struct PPM_Pixel {
    //Create variables to hold the rgb pixel values
    int red, green, blue;
} PPM_Pixel;

struct PPM *getPPM(FILE * fd){

    struct PPM *image;
    char buffer[16];
    char c;
    char commentArray[256];
    fd = fopen(fd, "r");

    struct PPM *newPPM = malloc(sizeof(PPM));

    //checks if the file being read is valid/the correct file
    if(fd == NULL){
        exit(1);
    }

    //read the image of the format
    if(!fgets(buffer, sizeOf(buffer), fd)){
        exit(1);
    }

    //checks the format of the ppm file is correct
    if(buffer[0] != 'P' || buffer[1] != '3'){
        fprintf(stderr, "Invalid image format! \n");
        exit(1);
    }else {
        printf("%s", newPPM->format);
    }


    //allocate memory for the image
    image = malloc(sizeof(PPM));
    if(!image){
        fprintf(stderr, "Cannot allocate memory");
        exit(1);
    }

    //checks whether the next character is a comment and store it in a linked list
    c = getc(fd);
    while(c == '#'){
        while(getc(fd) != '\n'){
        c = getc(fd);
        }
    }

    if(fscaf(fd, "%d %d", &image->height, &image->width)!= 2){
         fprintf(stderr, "Size of image is invalid");
         exit(1);
    }

    fclose(fd);
    return newPPM;
 };

showPPM(struct PPM * image){

    //two-dimensional array to show the rows and columns of pixels.

    int rgb_array[MAX_HEIGHT][MAX_WIDTH];
    int i;
    int j;

    for(i = 0; i<MAX_HEIGHT; i++){
        for(j = 0; j<MAX_WIDTH; j++){
            printf("%d", rgb_array[i][j]);
        }
    }



};

Answer 1

the following code

1. contains lots of comments about what is wrong and/or how to correct
2. performs appropriate error checking
3. corrects some function signatures
4. properly declares a prototype for each function
5. corrects some of the 'keypunch' errors
6. corrects the 'syntax' errors
7. moved the struct definitions to before those definitions are first referenced

Caveat: this code does not correct the problems with accessing the PPM image file.

Caveat: this code still does not pass the malloc'd memory areas to free(), so those statements still need to be added.

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


#define MAX_HEIGHT 600
#define MAX_WIDTH 400

// 1) giving the struct tag name the same as an instance name is a bad idea
// 2) good programming practice is to define a struct separately
//    from the instance of that struct

struct PPM
{
    char format[2]; //PPM format code
    int height, width; //image pixel height and width
    int max; //max rgb colour value

};

struct PPM myPPM;

struct PPM_Pixel
{
    //Create variables to hold the rgb pixel values
    //int red, green, blue;
    // follow the axiom:
    //    only one statement per line and (at most) one variable declaration per statement
    int red;
    int green;
    int blue;
};

struct PPM_Pixel myPPM_Pixel;

// prototypes
struct PPM *getPPM(FILE * fp);
void showPPM(struct PPM * image);

// this line will cause the compiler to raise two warnings 
//    about unused variable 'argc' and 'argv'
//int main(int argc, char ** argv)
int main( void )
{
    // get image file
    //FILE *file;
    //file = fopen("aab(1).ppm", "r");

    //if there is no file then return an error
    //if(file == NULL)
    FILE *fp == NULL;
    if( NUL == (fp = fopen("aab(1).ppm", "r") ) )
    {
        //fprintf(stderr, "File is not valid");
        //  should include system error message
        perror( "fopen for read of aab(1).ppm failed" );
        //return 0;
        // need to indicate an error occurred
        exit( EXIT_FAILURE );
    }

    // implied else, fopen was successful

    // following statement will raise a compiler warning
    //    about: 'set but not used variable'
    struct PPM *newPPM = getPPM(fp);

    fclose( fp ); // moved to here from 'getPPM()'
} // end function: main




struct PPM *getPPM(FILE * fp)
{
    struct PPM *image;
    char buffer[16];

    //char c;
    // the returned value from 'getc()' is an integer, not a character
    int  c;
    char commentArray[256];

    // following line is nonsence and the passed in parameter is already open
    //fd = fopen(fd, "r");

    // when allocating memory, always check the returned value
    //    to assure the operation was successful
    struct PPM *newPPM = NULL;
    if( NULL == (newPPM = malloc( sizeof struct PPM ) ) )
    {
        perror( "malloc for struct PPM failed" );
        exit( EXIT_FAILURE );
    }

    // implied else, malloc successful

    // already done in main()
    //checks if the file being read is valid/the correct file
    //if(fd == NULL){
    //    exit(1);
    //}

    //read the image of the format
    if(!fgets(buffer, sizeOf(buffer), fd)){
        exit(1);
    }

    //checks the format of the ppm file is correct
    if(buffer[0] != 'P' || buffer[1] != '3'){
        fprintf(stderr, "Invalid image format! \n");
        exit( EXIT_FAILURE );
    }

    // implied else, file format correct

    printf("%s", newPPM->format);



    //allocate memory for the image
    // note: there is no item named 'PPM'
    // image = malloc(sizeof(PPM));
    //if(!image){
    // best to keep things together
    if( NULL == (image = malloc( sizeof struct PPM ) ) )
    {
        //fprintf(stderr, "Cannot allocate memory");
        // should include the OS message for the failure
        perror( "malloc for struct PPM failed" );
        exit( EXIT_FAILURE );
    }

    // implied else, malloc successful


    // the following code does not store a comment in a linked list
    // and
    // skips over 1/2 the characters in the line due to the double call
    // to 'getc()' in the inner while loop
    //checks whether the next character is a comment and store it in a linked list
    //c = getc(fd);
    //while(c == '#'){
    //    while(getc(fd) != '\n'){
    //    c = getc(fd);
    //    }
    //}

    // this line does not compile!  did you actually mean to call: 'fscanf()'?
    //if(fscaf(fd, "%d %d", &image->height, &image->width)!= 2)
    if( 2 == fscanf( fp, "%d %d", &image->height, &image->width) )
    {
        // next line NOT true, what actually happened is the call to 'fscanf()' failed
        //fprintf(stderr, "Size of image is invalid");
        perror( "fscanf for image height and width failed" );
        exit( EXIT_FAILURE );
    }

    // implied else, fscanf successful

    // best practice to close a file, where it was opened.
    //fclose(fd);
    return newPPM;
 //};  do not follow a function definition with a semicolon
}


// Note: the following function is never called
// showPPM(struct PPM * image) each function signature needs a return type, even if 'void'
void showPPM(struct PPM * image)
{
    // Note: PPM images have the number of pixels in each row rounded up to a multiple of 4
    //       but this posted code fails to handle that
    //two-dimensional array to show the rows and columns of pixels.
    // bad idea, suggest passing the (already input) width and height
    //   and use those passed in values to define the array sizes
    int rgb_array[MAX_HEIGHT][MAX_WIDTH];
    int i;
    int j;

    for(i = 0; i<MAX_HEIGHT; i++)
    {
        for(j = 0; j<MAX_WIDTH; j++)
        {
            printf("%d", rgb_array[i][j]);
        }
    }
// }; do not follow a function definition with a semicolon
}