How to redesign this code to avoid goto

Question

I have a programme that needs the user to input 4 files (no order require). Then I do something to the files differently. Now I used the goto statement, I want to replace the goto statement, but don't know how. And I want to know if these go to have some problem? Here is my code that using goto:

int main(int argc, char **argv){
    char *tmp;
    int i, flag1=0, flag2=0, flag3=0, flag4=0;
    FILE *fp1;
    FILE *fp2;
    FILE *fp3;
    FILE *fp4;

    char file1[64];
    char file2[64];
    char file3[64];
    char file4[64];

    for( i=1; i<argc; i++){
        tmp = argv[i];
        if ( strcmp(tmp+8,"F1") == 0 ){
            sprintf(file1,argv[i]);
            flag1=1;
        }
        else if (strcmp(tmp+8,"F2") == 0 ){
            sprintf(file2,argv[i]);
            flag2=1;
        }
        else if (strcmp(tmp+8,"F3") == 0 ){
            sprintf(file3,argv[i]);
            flag3=1;
        }
        else if (strcmp(tmp+8,"F4") == 0 ){
            sprintf(file4,argv[i]);
            flag4=1;
        }
    }

    if( !(flag1 && flag2 && flag3 && flag4) ){
        printf("Must input four files!!\n");
        exit(-1);
    }

    if (access(file1,0) != 0){
        GOTO L1;
    }
    if((fp1 = fopen(file1,"r")) == NULL){
        exit(-1);
    }
    do_file_1(fp1);
    fclose(fp1);

    L1: if (access(file2,0) != 0 ){
        goto L2;
    }
    if((fp2 = fopen(file2,"r")) == NULL){
        exit(-1);
    }
    do_file_2(fp2);
    fclose(fp2);


    L2: if (access(file3,0) != 0)
    {
       goto L3;
    }
    if((fp3=fopen(file3,"r"))==NULL)
    {
      exit(-1);
    }
    do_file_3(fp3);
    fclose(fp3);



    L3: if (access(file4,0) !=0)
    {
      goto end;
    }
    if((fp4=fopen(file4,"r"))==NULL)
    {
      exit(-1);
    }
    do_file_4(fp4);
    fclose(fp4);


    end:
        return 0;
}

Use functions, now, code is unreadable. But if u want save code as now but without `goto` then I'll post. — Denis Sologub, May 17 '16 at 22:45
I don't see much point in making the program caller pass in file names that have to be named "F1", "F2", "F3", and "F4". — jxh, May 18 '16 at 00:10
the actually file name like this 20150518F1 20150518F2 20150518F3 20150518F4,the date change everyday — Tee, May 18 '16 at 15:03
The code in your question won't compile. You need `#include ` and `#include ` at the top, and `GOTO L1;` needs to be `goto L1;`. Also, the `sprintf` calls are dangerous; you're using `argv[i]` as a format string, and if it contains `%` characters Bad Things Can Happen. — Keith Thompson, Jul 20 '16 at 01:04

score 2 · Accepted Answer · answered May 17 '16 at 22:49

You have "if this condition is true, skip over some code". That's the only thing you're using goto for.

That's exactly what if does (except if skips the code if the condition is false).

You can replace:

L2: if (access(file3,0) != 0)
{
   goto L3;
}
if((fp3=fopen(file3,"r"))==NULL)
{
  exit(-1);
}
do_file_3(fp3);
fclose(fp3);

L3:

with:

if (access(file3,0) == 0)
{
  if((fp3=fopen(file3,"r"))==NULL)
  {
    exit(-1);
  }
  do_file_3(fp3);
  fclose(fp3);
}

and similarly for the other uses of goto.

score 2 · Answer 2 · edited May 23 '17 at 12:14

You refactor in steps.

You can take the end label statement and put it directly where it is called. That's easy.

For the others, you can use else statements:

if (access(file1,0) != 0){
    //GOTO L1;
}else{
  if((fp1 = fopen(file1,"r")) == NULL){
      exit(-1);
  }
  do_file_1(fp1);
  fclose(fp1);
}

L1: if (access(file2,0) != 0 ){
        //goto L2;
    }else{
      if((fp2 = fopen(file2,"r")) == NULL){
          exit(-1);
      }
      do_file_2(fp2);
      fclose(fp2);
    }

    L2: if (access(file3,0) != 0)
    {
       //goto L3;
    }else{
      if((fp3=fopen(file3,"r"))==NULL)
      {
        exit(-1);
      }
      do_file_3(fp3);
      fclose(fp3);
    }



    L3: if (access(file4,0) !=0)
    {
        end:
        return 0;
    }
    if((fp4=fopen(file4,"r"))==NULL)
    {
      exit(-1);
    }
    do_file_4(fp4);
    fclose(fp4);

Of course, since there is no other statements in the if statements, the if else can be refactored to just be an if.

I have broken down more complex problems like this in the past.

score 1 · Answer 3 · edited May 23 '17 at 11:52

Perhaps an approach like the following which uses loops with arrays would be shorter and more compact. This is a fairly straightforward transformation that consolidates separate variables into arrays. This does use an array of function pointers (see How do Function pointers in C work).

The do_file_1(), do_file_2(), etc. functions are just place holders for the actual functions you are using. This keeps the literal file names ("F1", "F2", etc.) that you specify though I am not sure why you are requiring certain, specific file names.

I also use the strcpy() function to copy the file name argument into the array of file names to open rather than using the sprintf() function.

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

void do_file_1 (FILE *fp)
{
    printf ("function do_file_1 called.\n");
}
void do_file_2 (FILE *fp)
{
    printf ("function do_file_2 called.\n");
}
void do_file_3 (FILE *fp)
{
    printf ("function do_file_3 called.\n");
}
void do_file_4 (FILE *fp)
{
    printf ("function do_file_4 called.\n");
}

int main(int argc, char **argv)
{
    int   i;
    char  *fileArray[4] = { "F1", "F2", "F3", "F4"};
    char  fname[4][64] = {0};
    void  (*funcs[4]) (FILE *fp) = {do_file_1, do_file_2, do_file_3, do_file_4};

    for( i = 1; i < argc; i++) {
        int  j;
        for (j = 0; j < 4; j++) {
            if ( strcmp(argv[i], fileArray[j]) == 0 ) {
                strcpy(fname[j],argv[i]);
                break;
            }
        }
    }

    if( !(fname[0][0] && fname[1][0] && fname[2][0] && fname[3][0]) ) {
        printf("Must input four files!!\n");
        exit(-1);
    }

    for (i = 0; i < 4; i++) {
        if (access(fname[i],0) == 0) {
            FILE *fp = fopen(fname[i],"r");
            if (fp != NULL) {
                funcs[i](fp);
                fclose(fp);
            } else {
                break;
            }
        } else {
            break;
        }
    }
    if (i < 4) exit(-1);

    return 0;
}

the actually file name like this 20150518F1 20150518F2 20150518F3 20150518F4,is used for bank reconciliation every day.strcmp(tmp+8,F1) and so on.hope this will make you clear:) — Tee, May 18 '16 at 14:57
I think your code is very cool and beautiful !! but immibis's answer is more clear here. — Tee, May 18 '16 at 15:32
@Tee, happy to hear your opinion of the code and this is a rewrite of your source example that is more condensed using more advanced features of the C programming language. As for the function arguments not being the same, I was going by your example which all have an argument of `FILE *` with a handle to the open file to be processed. — Richard Chambers, May 18 '16 at 15:35
if the function arguments not being the same is there a way to use an array of function pointers still? — Tee, May 18 '16 at 15:45
@Tee the question would be what are the differences between the function interfaces or list of arguments and their types. When you say function arguments not being the same do you mean different types in the function arguments (e.g. `int func_1 (char *p, int j)` versus `char * func_2 (int one, int two, float three)`). If by different arguments you mean the types in the interface and the number of arguments are the same but the actual values in the arguments are different then you could. If the function interface is different, then maybe depending on the differences. — Richard Chambers, May 18 '16 at 17:49

score 0 · Answer 4 · answered May 17 '16 at 22:49

the obvious variant is to change if statements like this:

if (access(file1,0) == 0){
    if((fp1 = fopen(file1,"r")) == NULL) exit(-1);
    do_file_1(fp1);
    fclose(fp1);
}

if (access(file2,0) == 0 ){
    if((fp2 = fopen(file2,"r")) == NULL) exit(-1);
    do_file_2(fp2);
    fclose(fp2);
}

...

score 0 · Answer 5 · answered May 17 '16 at 22:50

You're just using goto's as a way to reverse the if clauses. Reverse them directly and indent what lies between:

if (access(file1,0) != 0){
    GOTO L1;
}
if((fp1 = fopen(file1,"r")) == NULL){
    exit(-1);
}
do_file_1(fp1);
fclose(fp1);

L1: ...

Becomes:

if (access(file1,0) == 0) 
{
    if((fp1 = fopen(file1,"r")) == NULL){
        exit(-1);
    }
    do_file_1(fp1);
    fclose(fp1);
}
//L1:

score 0 · Answer 6 · answered May 17 '16 at 22:58

int main(int argc, char **argv){
    char *tmp;
    int i, flag1=0, flag2=0, flag3=0, flag4=0;
    FILE *fp1;
    FILE *fp2;
    FILE *fp3;
    FILE *fp4;

    char file1[64];
    char file2[64];
    char file3[64];
    char file4[64];

    for( i=1; i<argc; i++){
        tmp = argv[i];
        if ( strcmp(tmp,"F1") == 0 ){
            sprintf(file1,argv[i]);
            flag1=1;
        }
        else if (strcmp(tmp,"F2") == 0 ){
            sprintf(file2,argv[i]);
            flag2=1;
        }
        else if (strcmp(tmp,"F3") == 0 ){
            sprintf(file3,argv[i]);
            flag3=1;
        }
        else if (strcmp(tmp,"F4") == 0 ){
            sprintf(file4,argv[i]);
            flag4=1;
        }
    }

    if( !(flag1 && flag2 && flag3 && flag4) ){
        printf("Must input four files!!\n");
        return -1;
    }

    if (access(file1,0) == 0) {

        if((fp1 = fopen(file1,"r")) == NULL){
            return -1;
        }

        do_file_1(fp1);
        fclose(fp1);
    }
    /*L1:*/
    if (access(file2,0) == 0 ) {

        if((fp2 = fopen(file2,"r")) == NULL){
            return -1;
        }

        do_file_2(fp2);
        fclose(fp2);
    }

    /*L2:*/
    if (access(file3,0) == 0) {

       if((fp3=fopen(file3,"r"))==NULL) {
           return -1;
        }

        do_file_3(fp3);
        fclose(fp3);
    }

    /* L3: */
    if (access(file4,0) == 0) {

       if((fp4=fopen(file4,"r"))==NULL) {
           return -1;
       }

       do_file_4(fp4);
       fclose(fp4);
    }

    return 0;
}

But really, no one programer will not do so.

How to redesign this code to avoid goto

6 Answers6