Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am looking for advice on making the following code shorter. Any help is appreciated.

public String determineWinner(String winner) {
    winner = "";
    if (playChoice.equals(compChoice))
        winner = "The result is a tie.";
    else if (compChoice.equals("R"))
    if (playChoice.equals("S") || playChoice.equals("s"))
        winner = "Computer wins because Rock beats Scissors.";
    else
        winner = "Player wins because Paper beats Rock";
    else if (compChoice.equals("S"))
    if (playChoice.equals("P") || playChoice.equals("p"))
        winner = "Computer wins because Scissors beats Paper.";
    else
        winner = "Player wins because Rock beats Scissors";
    else 
    if (playChoice.equals("R") || playChoice.equals("r"))
        winner = "Computer wins because Paper beats Rock.";
    else
        winner = "Player wins because Scissors beats Paper.";
    return winner;
}
share|improve this question
1  
A very simple improvement would be using String.equalsIgnoreCase() in order to avoid checking twice for uppper and lower case. –  Landei Dec 12 '12 at 13:59
add comment

4 Answers 4

You code looks unclear from point of view of parameter usage and ignoring case difference is case of "The result is a tie.", so ...

Try this :).

public class testik {
    // No throws clause here
    public static void main(String[] args) {
        // Paper < Rock < Scissors < Paper
        System.out.println("Comp:S vs User:R = " + determineWinnerOtherWay("S","R")); //user win
        System.out.println("Comp:S vs User:P = " + determineWinnerOtherWay("S","P")); // com win

        System.out.println("Comp:P vs User:R = " + determineWinnerOtherWay("P","R")); //com win
        System.out.println("Comp:P vs User:S = " + determineWinnerOtherWay("P","S")); // user win

        System.out.println("Comp:R vs User:S = " + determineWinnerOtherWay("R","S")); //com win
        System.out.println("Comp:R vs User:P = " + determineWinnerOtherWay("R","P")); // user win
    }

    public static String code2Title(String code){
        if ("R".equals(code)) {
            return "Rock";
        } else if ("P".equals(code)) {
            return "Paper";
        } else {
            return "Scissors";
        }
    }

    public static String determineWinnerOtherWay(String compChoice, String playChoice) {
        String winner = "";
        String playerChoiceN = playChoice.toUpperCase();
        if (playChoice.equals(compChoice))
            winner = "The result is a tie.";
        int diff = compChoice.charAt(0) - playerChoiceN.charAt(0);
        if ( diff == 1 || diff == 2 || diff ==-3) {
            winner = "User win bacause:"+code2Title(playerChoiceN)+" wins " + code2Title(compChoice);
        } else {
            winner = "Computer wins because:"+code2Title(compChoice)+" wins " + code2Title(playerChoiceN);
        }
        return winner;
    }

}
share|improve this answer
    
A better choice, instead of compChoice.charAt(0) - playerChoiceN.charAt(0) (which is sort of hacky), would be to create a custom implementation of Comparator –  Clockwork-Muse Dec 12 '12 at 23:16
    
just tried to be close to task "making the following code shorter." :) but I agree with you. –  Roman Ivanov Dec 13 '12 at 4:13
add comment

Is this your homework assignment? Here's my solution in Python.

def winner(choice_P, choice_C):
    """ Given choices of Player and Computer (one of ['P', 'R', 'S']),
        return description of result of Rock, Paper, Scissors game."""
    names = ['Player', 'Computer']
    choices = ('Paper', 'Rock', 'Scissors')
    choice_pair = (choice_P + choice_C).upper()
    if choice_pair[0] == choice_pair[1]:
        return "The result is a tie."
    order = 'PRSP'
    index = order.find(choice_pair)
    if index == -1:                                 # not found
        names.reverse()
        index = order.find(choice_pair[::-1])       # (reversed)
        if index == -1:
            return "Invalid input, choices must be one of ['P', 'R', 'S']"
    return '%s wins because %s beats %s' % (names[0], 
                                            choices[index], choices[(index + 1) % 3])
share|improve this answer
add comment

I've written this in C#, but it should translate to Java easily.

I made determineWinner() call itself, reversing the order of player1 & player2. This eliminates fully half of the 'if else` maze.

// these are class level variables.
string winnerNotice = @"{0} wins. {1} beats {2}";
string drawNotice = @"Draw. Both players picked {0}";

// new structures to help clean up the determineWinner() method
public class Player { 
    public string Name;
    public choices choice;
}

public enum choices
{
    Paper = 1 , Scissors = 2 , Rock = 4
}

        public String determineWinner( Player player1, Player player2 ) {

            //tie
            if ( player1.choice == player2.choice ) 
                return string.Format( drawNotice ,player1.choice );

            // paper covers rock
            if ( player1.choice == choices.Paper && player2.choice == choices.Rock) 
                return string.Format(winnerNotice,player1.Name,player1.choice,player2.choice);

            // rock beats scissors beats paper
            if ( player1.choice > player2.choice ) 
                return string.Format( winnerNotice ,player1.Name ,player1.choice ,player2.choice );

            // player 1 did not win, did player 2 win?
            determineWinner(player2, player1);
        }
    }




}
share|improve this answer
add comment

Consider:

paper = 2
scissors = 3
rock = 5

Then your function could look like:

public int determineWinner(int player1, int player2)
{
    if(player1 != player2)
        if(player1 + player2 == 7)
            return Math.min(player1, player2);
        else
            return Math.max(player1, player2);
    else
        return 0;
}

Then the implementation is up to you.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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