CODEWITHSUNDEEP
cgoto
Joe Crawley
Joe Crawley

Reputation: 725

goto statements and alternatives in C

The program below is supposed to find the first all-zero row, if any, of an n × n matrix. Is this a correct approach to the problem, and is there another good structured approach to this code in C, or in any language in general? I'm trying to learn more about goto statements and appropriate uses/ alternatives to them. I appreciate any help!

int first_zero_row = -1; /* none */
int i, j;
for (i = 0; i < n; i++) {
    for (j = 0; j < n; j++) {
        if (A[i][j]) goto next;
    }
    first_zero_row = i;
    break;
    next: ;
}

Upvotes: 4

Views: 4970

Answers (7)

SomeGuy
SomeGuy

Reputation: 1

Since I've used my first goto to break out of nested loops in...well...ever, today, I'll weigh in on this. I felt bad about it so I read up a bit on why goto is considered evil.

The reason Dijkstra (and others) consider goto as evil is that code is built up in blocks where each block has preconditions and postconditions. A goto (or break) means that you cannot guarantee that a code block will generate a valid postcondition for the next block as precondition ( i.e. you cannot 'prove your code correct' since the break/goto may skip essential parts of the code block)

HOWEVER, the reason why I think goto is fine in your example is: You're actually setting up a fully valid postcondition (first_zero_row = -1 ...i.e.: "no all-zero row found") up front. So there is no chance that your block will go into an invalid postcondition state.

The issue of readability is another thing, though. Goto makes code rather unreadable. So for your particular example there may be better alternatives.

Upvotes: 0

Earth Engine
Earth Engine

Reputation: 10436

In my opinion, ALL GOTO IS EVIL. If in any case you would need them, you'd better consider "Extract Method" refactoring.

The need to use "GOTO" is a very good indicator shows your method (or function) is too complicated that you should split it into different pieces.

For example, the given example of the question can be (and should be) refactored into:

bool is_zero_row(int *row, int size){
    for(i = 0; i<size; ++i){
        if(row[i]) return false;
    }
    return true;
}

bool find_zero_row_in_array(int **A, int rows, int row_size){
    int i,j;
    for(i = 0; i < rows; ++i) {
        if(is_zero_row(A[i], row_size)) return true;
    }
    return false;
}

...
if(find_zero_row_in_array(A, n, n)){
    //Found
} else {
    //Not found
}
....

This is far more flexible and readable than the original. At least we now have a function that can work for any arrays with different numbers of rows and columns, rather assume they are the same.

Upvotes: -2

Omkant
Omkant

Reputation: 9204

You can use one variable which will work as flag. For if (A[i][j]) goto next; condition modify flag value to some other instead of goto next.

And then check that flag value later to do other operations , which is after next label in your case.

int flag = 0;
for ( i = 0; i < n; i++ ) {
    for ( j = 0; j < n; j++ ) {
        if ( A[i][j] ){
            flag = 1;
            break;
        }
    }
    if( flag == 1 ){
        // do your stuff 
        continue;
    }
    if( flag == 0 ){
        first_zero_row = i;
        break;
    }

NOTE : To remove goto from your code It may require you to adjust the position of some lines of code.

Upvotes: 0

kmkaplan
kmkaplan

Reputation: 18960

Your code looks quite readable to me. Indiscriminate witch-hunting of goto is not good. Go and read Donald E. Knuth’s Structured Programming with go to Statements for interesting thougts on the subject.

My version with goto, for maximum efficiency and quite readable too:

int i, j;
for (i = 0; i < n; i++) {
    for (j = 0; j < n; j++)
        if (A[i][j]) goto not_this_row;
    goto row_found;
    not_this_row:
}
/* Not found case here */

row_found:
/* Found case, i is the first zero row. */

Upvotes: 1

Bo Persson
Bo Persson

Reputation: 92261

What about using a little helper function to simplify the code in the loop:

bool is_zero_row(int* Row, int size)
{
    int j;

    for (j = 0; j < size; j++)
        if (Row[j] != 0)
            return false;

    return true;
}

and then

int first_zero_row = -1; /* none */
int i;
for (i = 0; i < n; i++) {
    if (is_zero_row(A[i], n))
    {
        first_zero_row = i;
        break;
    }
}

Upvotes: 4

melpomene
melpomene

Reputation: 85767

First off: Yes, your approach looks correct to me. You could rewrite it like this:

int first_zero_row = -1; /* none */
int i, j;
for (i = 0; i < n; i++) {
    int all_zeroes = 1;
    for (j = 0; j < n; j++) {
        if (A[i][j]) {
            all_zeroes = 0;
            break;
        }
    }
    if (all_zeroes) {
        first_zero_row = i;
        break;
    }
}

but I think that's actually less clear than the goto version. In a language like Perl that provides loop labels, you could do this:

my $first_zero_row = -1;  # none
ROW:
for my $i (0 .. $n-1) {
    for my $j (0 .. $n-1) {
        next ROW if $A[$i][$j];
    }
    $first_zero_row = $i;
    last ROW;
}

Upvotes: 1

iabdalkader
iabdalkader

Reputation: 17312

I'm not against using goto in some situations, but I think this can be re-written like this:

for (i = 0; i < n; i++) {
    for (j = 0; j < n; j++) {
        if (A[i][j]) break;
    }
    if (j==n) { /* the row was all zeros */
        first_zero_row = i;
        break;
    }
}

Upvotes: 4

Related Questions