CODEWITHSUNDEEP
c
Tee
Tee

Reputation: 93

How to redesign this code to avoid goto

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;
}

Upvotes: 1

Views: 120

Answers (6)

Laurel
Laurel

Reputation: 6173

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.

Upvotes: 2

Richard Chambers
Richard Chambers

Reputation: 17583

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;
}

Upvotes: 1

Denis Steinman
Denis Steinman

Reputation: 7799

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.

Upvotes: 0

aghast
aghast

Reputation: 15310

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:

Upvotes: 0

Criticize SE actions means ban
Criticize SE actions means ban

Reputation: 58868

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.

Upvotes: 2

Iłya Bursov
Iłya Bursov

Reputation: 24146

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);
}

...

Upvotes: 0

Related Questions