Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Hey, I am a beginner...I wrote this code about finding the end of file (EOF) It is written in C language! Help me make it correct..Anyone?? I want your reviews about this code! :) Thanx!

#include<stdio.h>
#include<conio.h>
void main (void)
{
int x,y,m;
for(x=0;x>=0;x++){
    m=scanf("%d %d",&x,&y);
    if (m!=2 || m==EOF){
    break;
    }
    else printf("/%d/%d/\n",x,y);
}
if (feof ( stdin )){
  printf("End of input\n");
}
else if(m!=2){
  printf("There was an error\n");
}
getch();
}
share|improve this question

1 Answer 1

Let me look at this...first you include the headers, good.

void main (void)
{
    /* snip */
}

Well, for C it's standard to call int main(int argc, char** argv) to facilitate arguments, or if there are no arguments just int main().

This is minor style compaint, not a major problem.

Examining your main function's contents:

    int x,y,m;
    for(x=0;x>=0;x++){
        m=scanf("%d %d",&x,&y); /* NOTE THIS LINE!!! */
        if (m!=2 || m==EOF){
            break;
        }
        else printf("/%d/%d/\n",x,y);
    }

1. This scanf uses STDIN for input, i.e., the user will enter stuff from the command line. Is this what you desire?

I don't think EOF would be entered by the user...

Perhaps you meant fscanf to scan a file for input?

2. So if I enter -9 3 I should expect this for-loop to terminate. Is this the desired behaviour?

On the /* NOTE THIS LINE!!! bit of code, I am suspicious about using sanf(...,&x,...). Again, perhaps this is desired, I don't know.

3. You should also be consistent with the use of brackets for single lines, writing if (...) break; instead. Or change the else to include brackets. But don't include brackets for one, then forget them for the other.

    if (feof ( stdin )){
        printf("End of input\n");
    }
    else if(m!=2){
        printf("There was an error\n");
    }
    getch();
}

4. These lines could be tucked into the for-loop.

5. Avoid using getch() since it's deprecated. If you're just waiting for the user to hit some key to continue, use getchar().

You could have condensed the main function to be:

int main() {
    int m,x,y;
    for(x=0; x>=0; x++) {
        m=scanf("%d %d",&x,&y);
        if (m!=2) { /* note multiple lines always trapped between brackets */
            printf("There was an error!\nHit any key to continue...\n");
            getchar();
            return EXIT_FAILURE;
        } else if (m==EOF) {
            printf("End of input\n");
            break();
        }
        else printf("/%d/%d/\n",x,y); /* single lines don't have brackets */
    }
    printf("Hit any key to continue...\n");
    getchar();
    return EXIT_SUCCESS;
}

The beauty of returning an int is you can have multiple different exit failures, e.g., -1 for bad input, -2 for some other problem, etc.

Addendum

After sleeping on it, I realized some additional style points for C.

A1. You should always initialize your variables! So instead of just writing int m,x,y; we should instead write:

    int m,x,y;
    x=0;
    y=0;
    m=0;

It's something people overlook in C programming.

A2. You could have written a while-loop instead of a for-loop, which may have been more elegant. For example:

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

int main() {
    int m,x,y;
    m = 0; 
    x = 0; 
    y = 0;
    while (x++ >= 0) {
        m=scanf("%d %d",&x,&y);
        if (m!=2) { /* note multiple lines always trapped between brackets */
            printf("There was an error!\nHit any key to continue...\n");
            getchar();
            exit(EXIT_FAILURE);
        } else if (m==EOF) break();
        else printf("/%d/%d/\n",x,y); /* single lines don't have brackets */
    }
    if (feof(stdin)) printf("End of input\n");
    printf("Hit any key to continue...\n");
    getchar();
    return EXIT_SUCCESS;
}

Note that we immediately terminate when there's an error, as is noted in the C Book in Chapter 1 and Chapter 10.

A3. Observe the only way to leave the while-loop is through (a) scanning an EOF, (b) scanning a "negative enough" number, or (c) through bad data. So we can notify the user of an error immediately, and exit (which is good form).

And the only remaining way to leave the loop is through (a) or (b). But it appears in the OP either case is acceptable, but only for (a) should we notify the user there's the end of input.

A4. Note upon exiting when encountering an error, it's good form to use exit(<error code>). You need stdlib.h to use exit(...) though.

share|improve this answer
    
The user can enter EOF for the input stream. On Unix it is done with <ctrl>-D On Windows <ctrl>-Z. See stackoverflow.com/a/10505004/14065 –  Loki Astari Dec 24 '12 at 0:16
    
These lines could be tucked into the for-loop. No they should obviously not be inside the for loop. They are a test to see if the data supplied in the file is correct. –  Loki Astari Dec 24 '12 at 3:13
    
@LokiAstari: thanks, although I contend for the exit failure case...you should stick them into the for-loop. At least, that's what I've read in the C books. However, I may be in error, and if that's the case, by all means correct me :) I'd be interested in any further remarks on my addendum. –  Alex Nelson Dec 25 '12 at 1:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.