1
\$\begingroup\$

How can I rewrite this code to make it more readable and, if possible, more efficient? The conditions are entirely necessary in this problem, but how could I rewrite those if(a != 1) and such?

celula ** verificaNo(celula ** matriz, int linhas, int colunas){
celula ** aux = matriz;
int i, j, k;
int node = 1;
int a = 0;
int b = 0;
int c = 0;


for(i = 0; i < linhas; i++){
    for(j = 0; j < colunas; j++){
        if((i == 0) && (j == 0)){
            aux[i][j].no = node;
        }
        else if((i == 0) && (j != 0)){
            if(aux[i][j-1].peso == 0 && aux[i][j].peso == 0){
                aux[i][j].no = aux[i][j-1].no;
            }
            else if(aux[i][j-1].peso != 0 && aux[i][j].peso == 0){
                for(k = j; aux[i+1][k].peso == 0 && k > 0; k--){
                    if(aux[i][k-1].peso == 0){
                        aux[i][j].no = aux[i][k-1].no;
                        a = 1;
                    }
                }
                if(a != 1){
                    node++;
                    aux[i][j].no = node;
                }
            }
        }
        else if((j == 0) && (i != 0)){
            if(aux[i-1][j].peso == 0 && aux[i][j].peso == 0){
                aux[i][j].no = aux[i-1][j].no;
            }
            else if(aux[i-1][j].peso != 0 && aux[i][j].peso == 0){
                for(k = j; aux[i][k].peso == 0 && k < colunas; k++){
                    if(aux[i-1][k+1].peso == 0){
                        aux[i][j].no = aux[i-1][k+1].no;
                        b = 1;
                    }
                }
                if(b != 1){
                    node++;
                    aux[i][j].no = node;
                }
            }
        }
        else if(aux[i][j].peso == 0 && aux[i-1][j].peso == 0 && i != 0){
            aux[i][j].no = aux[i-1][j].no;
        }
        else if(aux[i][j].peso == 0 && aux[i][j-1].peso == 0 && j != 0){
            aux[i][j].no = aux[i][j-1].no;
        }
        else if(aux[i][j].peso == 0){
            for(k = j; aux[i][k].peso == 0 && k < colunas; k++){
                if(i != 0){
                    if(aux[i-1][k].peso == 0){
                        aux[i][j].no = aux[i-1][k].no;
                        c = 1;
                    }
                }
            }
            if(c != 1){
                node++;
                aux[i][j].no = node;
            }
        }
    }
}   
return aux;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ As to discuss the readability, would you explain a little bit more about its function? So no one has to guess. \$\endgroup\$ – Wolf Nov 28 '13 at 15:06
  • \$\begingroup\$ If you wrote it in English, it would be more readable to me. ;-) \$\endgroup\$ – Fiddling Bits Dec 9 '13 at 23:07
6
\$\begingroup\$

This would become instantly more understandable if you gave a description (a comment at the start of the function) of what it was supposed to do. You could also give the structure celula, which I'm assuming is at least:

struct celula
{
    int no;        // number of something?
    int peso;      // weight, perhaps?
};

Also you use i for line (linhas) and j for column (colunas). But l and c would be more logical and readable (but l looks like 1, so in English I'd use r for "row"). I'm generally ok with short variable names, but in such a long function i and j don't work for me. In English I would probably use row and col.

As your function has nested loops with the outer loop doing nothing but enclosing the inner loop, I would if possible extract the whole inner loop to a separate function. But I can see that this might not be easy if those badly named variables a, b, c, loop accumulate across rows. It is difficult to tell whether they do from a short look - I might take a longer look later...

Later...

Returning to variables a, b, c: they are never reset once set. Is that correct? These variables names are meaningless and need changing. And what does node mean - again the name needs improving.

A bigger observation is that everything below the first else contains the term aux[i][j].peso == 0. I think these can all be factored out (so that the condition is tested at the top of the loop.

for(i = 0; i < linhas; i++){
    for(j = 0; j < colunas; j++){
        if((i == 0) && (j == 0)){
            aux[i][j].no = node;
        }
        else if(aux[i][j].peso == 0) {  // moved to here
            if((i == 0) && (j != 0)){

And also the i != 0 and j != 0 in the following lines are unnecessary, as they have been handled further up:

else if(aux[i-1][j].peso == 0 && i != 0){                     // here
    aux[i][j].no = aux[i-1][j].no;
}
else if(aux[i][j-1].peso == 0 && j != 0){                     //here
    aux[i][j].no = aux[i][j-1].no;
}
else{
    for(k = j; aux[i][k].peso == 0 && k < colunas; k++){
        if(i != 0){                                           // here
\$\endgroup\$
  • \$\begingroup\$ I'm guessing peso is the former Spanish currency, and no could either be Spanish for no (and use it as a kind of boolean) or it could be number of. \$\endgroup\$ – Max Nov 28 '13 at 14:45
  • \$\begingroup\$ Maybe peso is weight? \$\endgroup\$ – marcp Nov 28 '13 at 17:00
4
\$\begingroup\$

For readability this can help:

  • use the usual C-style short forms for 0-check if (i) and if if (!i), so the non-trivial comparisons get weight
  • add a space between keywords like if, for and the following opening parenthesis

Edit: [as usual] Use meaningful names, and, where not applicable (only!), comments.

\$\endgroup\$
3
\$\begingroup\$

Since you are passing matriz as a pointer, I don't think you need to create a local copy. This also means that your function doesn't need a return value. The matrix being passed in is the one being operated on.

There are 3 special cases here around the zero index, I would break them out into separate loops. There is also a repeated inner loop that can be extracted to a separate function,so I would do that. The first iteration counts down instead of up, though. I've preserved that as an option, but maybe its a typo?

Once you make those 2 changes there are a few simplifications that can be made. I used the boolean tests to get rid of all the ==0 and !=0, which cleans things up a bit.

EDIT: I've implemented @Roddy's suggestions

bool test(celula * *matriz, int &i, int &k, int incrementor int limit) {
     bool result = false;
     for (; !matriz[i + 1][k].peso && k > 0 && k<limit; k + incrementor) {
          if (!matriz[i][k - 1].peso ) {
                matriz[i][j].no = matriz[0][k - 1].no;
                result = true;
          }
     }
}

 void verificaNo(celula * *matriz, int linhas, int colunas) {
     int node = 1;

     // i, j == 0
     matriz[0][0].no = node;

     // i==0, j != 0
     for (int j = 1; j < colunas; ++j) {
         if (!matriz[0][j - 1].peso && !matriz[0][j].peso) {
             matriz[0][j].no = matriz[0][j - 1].no;
         } else if (matriz[0][j - 1].peso && !matriz[0][j].peso && !test(matriz, 0, j, -1, colunas)) {
             matriz[0][j].no = ++node;
         }
     }

     // j == 0, i != 0
     for (int i = 1; i < linhas; ++i) {
         if (!matriz[i - 1][0].peso && !matriz[i][0].peso) {
             matriz[i][0].no = matriz[i - 1][0].no;
         } else if (matriz[i - 1][0].peso && !matriz[i][0].peso == 0 && !test(matriz, i, 0, 1, colunas)) {
             matriz[i][0].no = ++node;
         }
     }

     // i > 0, j> 0
     for (int i = 1; i < linhas; ++i) {
         for (int j = 1; j < colunas; ++j) {
             if (!matriz[i][j].peso) {
                 if (!matriz[i - 1][j].peso) {
                     matriz[i][j].no = matriz[i - 1][j].no;
                 } else if (!matriz[i][j - 1].peso) {
                     matriz[i][j].no = matriz[i][j - 1].no;
                 }
             } else if (!matriz[i][j].peso && test(matriz, i, j, +1, colunas)) {
                 matriz[i][j].no = ++node;
             }
         }
     }
 }
\$\endgroup\$
2
\$\begingroup\$

Readability:

a, b, and c are not meaningful names, unless you have a very specific problem space.

Replace these

   node++;
   aux[i][j].no = node;

...with these

   aux[i][j].no = ++node;

And for C99 or later, use local for loop variables

  for (int j = 1; j < colunas; ++j) 

It's also good to get in the habit of using preincrement on loop variable (as above), as this can give greater efficiency particularly if you move to C++.

\$\endgroup\$
  • \$\begingroup\$ It's C, not C++. \$\endgroup\$ – Wolf Nov 29 '13 at 13:45
  • \$\begingroup\$ @Wolf Yes: for loop with declaration is valid C99. Or did you mean something else? \$\endgroup\$ – Roddy Nov 29 '13 at 16:59
  • \$\begingroup\$ Yes, and even const. But the OP didn't state that used C99. And since you mentioned also the performance preincrement, I wanted remind you about this. Is I read it again now, your have formulated this quite good. Maybe you could explicitly add the C99 vs. C89 distinction. There are people who are in the sad position to have to use non-standard compiler... \$\endgroup\$ – Wolf Nov 29 '13 at 17:37
  • 2
    \$\begingroup\$ @wolf : "the OP didn't state that used C99". No, but he didn't say "K&R C" either ;-) It's a reasonable to assume the most recent ratified standard unless otherwise specified. \$\endgroup\$ – Roddy Nov 29 '13 at 18:02
  • 2
    \$\begingroup\$ @chux : Yes. en.wikipedia.org/wiki/C11_(C_standard_revision) \$\endgroup\$ – Roddy Dec 9 '13 at 10:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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