1
\$\begingroup\$

I've created function that will display a league competition from the total team numbers.

For example if we have 4 teams it will display (a vs b) vs (c vs d) and if we have 5 teams it will diplay ((a vs b) vs c) vs (d vs e) and so on. It will display the competition looks square or equal.

here is the complete code :

public class EqualPlayer {
    private int index=0;

    public static void main(String[] args){
        EqualPlayer equalPlayer = new EqualPlayer();
        System.out.println(equalPlayer.getEqualPlayerID(26));
    }


    private String getEqualPlayerID(int numberPlayers){
        int nPDevided = numberPlayers/2;
        int A = numberPlayers-nPDevided;
        int B = nPDevided;
        String result="";

        if(A>2 || B > 2)
            return "( "+getEqualPlayerID(A)+" )"+" vs "+ "( "+getEqualPlayerID(B)+" )";

        if(A==2)
            if(result.isEmpty()){
                result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
                index+=2;
            }
            else{
                result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
                index+=2;
            }
        else if(A==1)
            if(result.isEmpty()){
                result = ""+getPlayerId(index);
                index+=1;
            }
            else{
                result = result+" vs "+ getPlayerId(index);
                index+=1;
            }

        if(B==2)
            if(result.isEmpty()){
                result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
                index+=2;
            }
            else{
                result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
                index+=2;
            }
        else if(B ==1)
            if(result.isEmpty()){
                result = ""+getPlayerId(index);
                index+=1;
            }
            else{
                result = result+" vs "+ getPlayerId(index);
                index+=1;
            }


        return result;

    }


    private char getPlayerId(int index){
        char [] alphabet = "abcdefghijklmnopqrstuvwxyz".toCharArray();
        return  alphabet[index];
    }

}

Any better idea how to create this function without recursive?

And also the limits of the player is 26 which is the total alphabets, if I want to increase the total player I need to add additional char to getPlayerId method, I would like to know about what are you guys think about that?
Improvement, suggestion, are welcome.

\$\endgroup\$
2
\$\begingroup\$

You can clean up your code by looking for similarities and getting rid of the duplication by creating separate functions. I'll demonstrate.


Let's take a look at this bit:

    if(A==2)
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    else if(A==1)
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }

    if(B==2)
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    else if(B ==1)
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }

Step 1 - add braces.

    if(A==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }
    }
    if(B==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
            index+=2;
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
            index+=2;
        }
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
            index+=1;
        }
        else{
            result = result+" vs "+ getPlayerId(index);
            index+=1;
        }
    }

Step 2 - identify sections of duplicate code and generalize them.

In your case, this is the index+=2 and index+=1 at the end of each case.

    if(A==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=2;
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=1;
    }
    if(B==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=2;
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=1;
    }

Next, further reduce the duplicate code. You can see that if B is 2, we'll add 2 to index. If B is 1, we'll add 1 to index. We can just add B to index. (We're reducing duplication of constants here)

    if(A==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=A;
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=A;
    }
    if(B==2){
        if(result.isEmpty()){
            result = "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
        }
        else{
            result =  result+" vs ( "+getPlayerId(index)+" vs "+ getPlayerId(index+1) +" )";
        }
        index+=B;
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=B;
    }

Next, remove duplication by creating a separate function that allows you to create the "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )" part in one call.

private String printVersus(int startIndex){
    return "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
}

Then, use it.

    if(A==2){
        if(result.isEmpty()){
            result = printVersus(index);
        }
        else{
            result = result+" vs "+printVersus(index);
        }
        index+=A;
    } else if(A==1) {
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=A;
    }
    if(B==2){
        if(result.isEmpty()){
            result = printVersus(index);
        }
        else{
            result =  result+" vs "+printVersus(index);
        }
        index+=B;
    } else if(B == 1){
        if(result.isEmpty()){
            result = ""+getPlayerId(index);
        }
        else{
            result = result+" vs "+ getPlayerId(index);
        }
        index+=B;
    }

Next, once again identify that there is duplication in handling of string appending - you handle result.isEmpty() in multiple places. Create a separate function. I gave it a bit of a poor name, since I haven't actually checked your business logic yet - I'm just optimizing this based on patterns!

private String appendVersusString(String original, String toAdd){
    if(original.isEmpty()){
        return toAdd;
    }
    else{
        return original+" vs "+ toAdd;
    }
}

Use the new function.

    if(A==2){
        result = appendVersusString(result, printVersus(index));
        index+=A;
    } else if(A==1) {
        result = appendVersusString(result, ""+getPlayerId(index));
        index+=A;
    }
    if(B==2){
        result = appendVersusString(result, printVersus(index));
        index+=B;
    } else if(B == 1){
        result = appendVersusString(result, ""+getPlayerId(index));
        index+=B;
    }

Once again, spot duplication between A and B - they're doing the exact same at the end of the case. I am assuming that A and B cannot go into the negatives.

Since if(A>2 || B > 2) has an early return, A and B are (individually) either 2, 1, or 0.

Thus we can see that if A is 2, we add A. If A is 1, we add A. If A is 0, we don't add anything. Same goes for B.

So we can just do index+=A in all three cases.

    if(A==2){
        result = appendVersusString(result, printVersus(index));
    } else if(A==1) {
        result = appendVersusString(result, ""+getPlayerId(index));
    }
    index+=A;
    if(B==2){
        result = appendVersusString(result, printVersus(index));
    } else if(B == 1){
        result = appendVersusString(result, ""+getPlayerId(index));
    }
    index+=B;

There is still duplication in the handling of A and B when it comes to appendVersusString. I think they're "groups", so I'll name the function handleGroup:

private String handleGroup(String result, int group, int currentIndex){
    if(group==2){
        result = appendVersusString(result, printVersus(currentIndex));
    } else if(group == 1) {
        result = appendVersusString(result, ""+getPlayerId(currentIndex));
    }
    return result;
}

And use your new function.

    handleGroup(result, A, index);
    index+=A;
    handleGroup(result, B, index);
    index+=B;

Final result:

public class EqualPlayer {
    private int index=0;

    public static void main(String[] args){
        EqualPlayer equalPlayer = new EqualPlayer();
        System.out.println(equalPlayer.getEqualPlayerID(26));
    }


    private String getEqualPlayerID(int numberPlayers){
        int nPDevided = numberPlayers/2;
        int A = numberPlayers-nPDevided;
        int B = nPDevided;
        String result="";

        if(A>2 || B > 2)
            return "( "+getEqualPlayerID(A)+" )"+" vs "+ "( "+getEqualPlayerID(B)+" )";

        handleGroup(result, A, index);
        index+=A;
        handleGroup(result, B, index);
        index+=B;

        return result;
    }

    private String handleGroup(String result, int group, int currentIndex){
        if(group==2){
            result = appendVersusString(result, printVersus(currentIndex));
        } else if(group == 1) {
            result = appendVersusString(result, ""+getPlayerId(currentIndex));
        }
        return result;
    }

    private String appendVersusString(String original, String toAdd){
        if(original.isEmpty()){
            return toAdd;
        }
        else{
            return original+" vs "+ toAdd;
        }
    }

    private String printVersus(int startIndex){
        return "( "+ getPlayerId(index) +" vs " +getPlayerId(index+1) +" )";
    }

    private char getPlayerId(int index){
        char [] alphabet = "abcdefghijklmnopqrstuvwxyz".toCharArray();
        return  alphabet[index];
    }

}

Lastly, you could consider using a StringBuilder to prevent needless copying of String contents. I also recommend putting spaces around operators to improve readability (if(A == 2) instead of if(A==2) and index += A instead of index+=A).

\$\endgroup\$
  • \$\begingroup\$ wow you're really awesome, thank you for answer sir!. I wish I could add 100 upvote for this \$\endgroup\$ – Gujarat Santana Oct 26 '16 at 11:52
  • \$\begingroup\$ and by the way is it possible solve this problem without using recursive? since I'm looking another possible solution for this also, I would love to hear your thought. \$\endgroup\$ – Gujarat Santana Oct 26 '16 at 11:54
  • 1
    \$\begingroup\$ @GujaratSantana Recursive is a very natural solution for this case. I know there's a non-recursive solution possible (using a loop and making use of the binary form of a number, but I don't feel like explaining myself fully (would take a lot of time). \$\endgroup\$ – Pimgd Oct 26 '16 at 12:05
  • \$\begingroup\$ @GujaratSantana look at binary trees and binary representation of a number, see if you can do anything interesting with "the number of players" and "how does a binary tree with <number of players> leaves look" \$\endgroup\$ – Pimgd Oct 26 '16 at 12:15
  • 1
    \$\begingroup\$ @GujaratSantana to be honest I don't have a good idea either, my recursive solution is making my own "call stack" which is ... bad - essentially recursive but all in the same function \$\endgroup\$ – Pimgd Oct 26 '16 at 12:25

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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