1
\$\begingroup\$

Is this good or can it be improved? The program should backtrack the two variables until this logic operation is true: a & b.

#include <stdio.h>
#include <stdbool.h>

bool perm(bool, bool);

int main(){

    bool x = false;
    bool y = false;

/*  RECURSIVE SOLUTION  */

    if(perm(x,y) == true){
        puts("DONE\n");
    }

    puts("\n\n");


/* ITERATIVE SOLUTION*/


    for(int cnt = 0; cnt < 2; cnt++){
        x = cnt;
        for(int cnt_2 = 0; cnt_2 < 2; cnt_2++){
            y = cnt_2;
            if( (x & y) == true){
                printf("%d ^ %d =  TRUE\n", x,y);
                puts("DONE");
                return 0;
            }
            printf("%d ^ %d =  FALSE\n", x,y);
        }
    }

    puts("NOTHING");

    return 0;
}


bool perm(bool x, bool y){

    const bool c_x = x;
    const bool c_y = y;

    printf("%d ^ %d  = %d\n", c_x, c_y, c_x&c_y);

    if(x == true){ return x;}
    if(y == true){ return y;}

    perm(y+1,x);

    return ( (x & y) == true) ? true : perm(x+1,y); 
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Please provide a better explanation of what this code accomplishes. \$\endgroup\$ – 200_success Mar 1 '18 at 3:33
  • 1
    \$\begingroup\$ Providing a use case, or usage example would be one way to illustrate to others what you normally would use this code for, giving insight into its application, and therefore more insight into how to make suggestions for improving. \$\endgroup\$ – ryyker Mar 1 '18 at 20:11
1
\$\begingroup\$

I find this code rather odd. I think there are a number of problems you can work out, which I'll outline below.

Functions

You put the recursive version into a function called perm() which is a good idea. I would do the same thing with the iterative version. This way your main() function becomes more clear. I'd rename the recursive one to recursive_perm() and make the new one iterative_perm(). Then your main() becomes:

int main(){

    bool x = false;
    bool y = false;

    if(recursive_perm(x,y) == true){
        puts("DONE\n");
    }

    puts("\n\n");

    x = false;
    y = false;
    if(iterative_perm(x,y) == true) {
        puts("DONE\n");
    }

    puts("NOTHING");

    return 0;
}

Now you don't need the comments saying "RECURSIVE SOLUTION" and "ITERATIVE SOLUTION" because it's obvious from the names.

Fix Your Recursion

You recursive version doesn't do the same thing as your iterative version. In fact, it does a lot of really weird things. Why are you copying the input variables into const members only to print them out? printf() takes its arguments by value, so printing them won't change their values locally.

Also, why are you printing logical XOR when you're performing logical AND? The ^ operator is XOR in C-based languages. Your print statement tells the user that the value of x exclusive-or-ed with y is equal to the value of x logically anded with y.

Next, why are you calling perm(y+1,x); before the return statement? You don't do anything with its return value, so none of the work it does is at all useful. You throw out the results (though it does have the side effect of printing out some stuff).

What do these incorrect lines accomplish?

if(x == true){ return x;}
if(y == true){ return y;}

They appear to check if only 1 of the values is true and then return true, ignoring the other parameter. Therefore it is not actually calculating the permutations that you want it to calculate.

Doing addition on bool values is not easy to read. I would not recommend doing that in the future. You can easily flip a boolean value from true to false by prepending it with the ! operator. Either that or change the function to take int instead of bool as you've done with the iterative version.

Testing bool

In general, it's considered better form to directly test a bool variable in an if statement rather than comparing it to true or false by doing this:

bool x = true;
bool y = false;
if (x) {
    // x was true, do something here
}
if (!y) {
    // y was false, do something here
}

What Does the Return Value Represent?

What does the return value of your perm() function represent? If all you want to do is iterate through all possible permutations, it's not clear to me that it needs to return a value. Reading this, I would assume that if it's functioning correctly, it iterated all the values, otherwise it wouldn't have returned yet. If you want to know which combination returned true, you should probably return a struct containing 2 values showing the final combination.

\$\endgroup\$
  • \$\begingroup\$ I want to know wich combination give this expression true, i can't use struct only basic operation. Thanks for your feedback it was very informative! \$\endgroup\$ – Robert Mar 1 '18 at 5:30

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.