1
\$\begingroup\$

I have a program that brute force searches for a combination of inputted array elements by the user and when the program is searching all the for loops are nested for the brute search then I have this test() which looks is responsible for testing out all the possible combinations of the nested for loop to find the the combination entered by the user of the program, how ever the code is super redundant, here is a sample.

public static boolean test(int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, int k){
    boolean result1 = true;


    if(a == 0){
        cube = turn_U(cube);
    }
    else if(a == 1){
        cube = turn_Ui(cube);
    }
    else if(a == 2){
        cube = turn_U2(cube);
    }
    else if(a == 3){
        cube = turn_L(cube);
    }
    else if(a == 4){
        cube = turn_Li(cube);
    }
    else if(a == 5){
        cube = turn_L2(cube);
    }
    else if(a == 6){
        cube = turn_F(cube);
    }
    else if(a == 7){
        cube = turn_Fi(cube);
    }
    else if(a == 8){
        cube = turn_F2(cube);
    }
    else if(a == 9){
        cube = turn_R(cube);
    }
    else if(a == 10){
        cube = turn_Ri(cube);
    }
    else if(a == 11){
        cube = turn_R2(cube);
    }
    else if(a == 12){
        cube = turn_B(cube);
    }
    else if(a == 13){
        cube = turn_Bi(cube);
    }
    else if(a == 14){
        cube = turn_B2(cube);
    }
    else if(a == 15){
        cube = turn_D(cube);
    }
    else if(a == 16){
        cube = turn_Di(cube);
    }
    else if(a == 17){
        cube = turn_D2(cube);
    }

This is what happens for letters a through k and I was wondering how I could shorten this, I had an idea where I could make a method that selects a turn method based on the letter in the test() , this would reduce so that test has 11 calls to the method instead of the all of this repeated ones.

\$\endgroup\$
5
  • \$\begingroup\$ If you want real advice on how to improve your code, you should post more than just this excerpt, because we can't really understand or improve your algorithm just by looking at this part of it. \$\endgroup\$ Jul 15, 2016 at 5:06
  • \$\begingroup\$ In fact, I would consider this question almost off-topic for lack of context. \$\endgroup\$ Jul 15, 2016 at 5:06
  • \$\begingroup\$ Nothing to review here. The code presented is not even a complete function! \$\endgroup\$
    – CiaPan
    Jul 15, 2016 at 8:19
  • \$\begingroup\$ here I will show you some more code. @CiaPan \$\endgroup\$ Jul 15, 2016 at 12:49
  • 2
    \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. I would recommend you ask a new question. \$\endgroup\$
    – Pimgd
    Jul 15, 2016 at 13:01

2 Answers 2

2
\$\begingroup\$

when the program is searching all the for loops are nested for the brute search

That sounds like the first thing to fix. The brute force search should use recursion or an iterative approach which builds up a data structure (a stack or queue).

then I have this test() which looks is responsible for testing out all the possible combinations of the nested for loop to find the the combination entered by the user of the program

Each of the six basic moves is a permutation. They could be represented as a two-dimensional array, and then a single method could apply an appropriate one by selecting the appropriate permutation from the array.

Moreover, the expanded moves are just iteration, and implementing them as iteration will give you much cleaner code.

public void applyMove(int move) {
    int iterations = (move % 3) + 1;
    int basicMove = move / 3;
    while (iterations-- > 0) {
        applyBasicMove(basicMove);
    }
}

public void applyBasicMove(int basicMove) {
    int[] permutation = basicPermutations[basicMove];
    ...
}

If you were worried about performance (which you clearly aren't, given that you're using a brute force search across a group of about \$4 \times 10^{19}\$ elements), you could optimise slightly by pre-computing the powers \$F^2\$ and \$F^3\$ (i.e. \$F^{-1}\$, or Fi if you prefer).

\$\endgroup\$
1
  • \$\begingroup\$ @Phrancis, \$F^i\$ makes no sense. \$\endgroup\$ Jul 16, 2016 at 8:30
1
\$\begingroup\$

Have you considered using a switch statement instead of a whole bunch of if / else if ? Java: The switch Statement

It would go like this:

switch (a) {
    case 0:
        cube = turn_U(cube);
        break;
    case 1:
        cube = turn_Ui(cube);
        break;
    case 2:
        cube = turn_U2(cube);
        break;
    // and so on...
    default:
        // you can write a default case here, akin to an `else` clause at the end, if you need to.
}

Of course this is not a complete solution, as it is a bit hard to tell exactly what the best solution would be without seeing the rest of your code. It should be at least enough to simplify those long chains of if/else branches.

Additional reference: Why switch is faster than if on Stack Overflow.


Additional note: Please come up with better names for the things in this method. Naming the method test doesn't say anything about what it is testing or why. Input parameters a through k don't say anything about what those parameters mean. result1 is equally ambiguous... result #1 of what?

\$\endgroup\$
1
  • \$\begingroup\$ I did add some more code to help the understanding part of the program. @Phrancis \$\endgroup\$ Jul 15, 2016 at 12:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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