OOP implementation of Rock Paper Scissors game logic in Java

Question

What is your opinion on my solution for Rock Paper Scissors game logic in Java?

Round outcome enum class

public enum Winer
{
    DRAW,
    PLAYER,
    COMPUTER;
}

Played moves enum class

public enum PlayedMove
{
    ROCK,
    PAPER,
    SCISSORS;
}

Game logic class

public class GameLogic
{
    public static Winer getWiner(PlayedMove player, PlayedMove computer)
    {
        if (player == computer)
        {
            return Winer.DRAW;
        }
        else if (isPlayerWiner(player, computer))
        {
            return Winer.PLAYER;
        }
        else
        {
            return Winer.COMPUTER;
        }
    }
    
    private static boolean isPlayerWiner(PlayedMove player, PlayedMove computer)
    {
        return ((player == PlayedMove.ROCK && computer == PlayedMove.SCISSORS)
             || (player == PlayedMove.SCISSORS && computer == PlayedMove.PAPER)
             || (player == PlayedMove.PAPER && computer == PlayedMove.ROCK));
    }
}

Answer 1

The code looks fine, few suggestions:

• Typo: winer should be winner.
• Naming: since the variable player is of type PlayedMove, it's better to name it playerMove. Same for the variable computer. Reading playerMove == PlayedMove.ROCK is betten than player == PlayedMove.ROCK. The class name GameLogic seems too generic, consider a more specific name, for example RockPaperScissors.
• Ternary operator: the method getWiner can use the ternary operator for deciding the winner between Winner.COMPUTER and Winner.PLAYER.
• Redundant parentheses: in the last return the outer parentheses can be removed.
• Code format: personally, I prefer the curly braces on the same line and IntelliJ Idea auto-formatting agrees with this rule.
• OOP missing: as others have mentioned, it's hard to say that this implementation is object-oriented since there are only static methods. To make it more object-oriented, there should be at least one object that stores some data as its own state and provides methods to manipulate such state.
• Minor readability improvement (optional): adding methods like isRock, isPaper and isRock to PlayedMove, makes the code more compact and a little bit easier to read. Alternatively, PlayedMove can be imported statically but I prefer the former.

Revised code:

public static Winner getWinner(PlayedMove playerMove, PlayedMove computerMove) {
    if (playerMove == computerMove) {
        return Winner.DRAW;
    } 
    return isPlayerWinner(playerMove, computerMove) ? Winner.PLAYER : Winner.COMPUTER;
}

private static boolean isPlayerWinner(PlayedMove playerMove, PlayedMove computerMove) {
    return (playerMove.isRock() && computerMove.isScissors())
            || (playerMove.isScissors() && computerMove.isPaper())
            || (playerMove.isPaper() && computerMove.isRock());
}

PlayedMove:

enum PlayedMove {
    PAPER(0),
    ROCK(1),
    SCISSORS(2);

    private final int value;

    PlayedMove(int value) {
        this.value = value;
    }

    public boolean isPaper() {
        return this.value == PAPER.value;
    }

    // isRock and isScissors methods..
}

Answer 2

I don't see anything object-oriented in it. A program without a single non-static method can hardly be called OO.

The main task of this piece of code is to compare PlayedMove instances, so to follow OO style, you should make that comparison a method of PlayedMove. This will make the enum a bit more complex (beyond beginner level), so you might want to switch away from PlayedMove being an enum (or, use it as an exercise on complex enums).

Answer 3

This is not OOP, because all your methods are static.

To make it into an OOP program, you could re-model it like this: A game has two players. For the game, it is not important, if the player is human or computer. Now, a human can only play against a computer, but what if you want to implement a multi-player mode?

First we create an interface for the player:

public interface Player
{
   PlayedMove chooseMove();
}

There should be two classes implementing the interface, one for a human and one for a computer:

public class HumanPlayer implements Player
{
   public PlayedMove chooseMove()
   {
       // read from keyboard or wait for button clicked
   }
}

public class ComputerPlayer implements Player
{
   public PlayedMove chooseMove()
   {
       // choose randomly
   }
}

Then you have a game class, which holds two players. If you call the play() method, it will every player chose who their move and then return the winning player, or null if it was a draw.

public class Game
{
   private Player player1;
   private Player player2;

   public Game(Player player1, Player player2)
   {
       this.player1 = player1;
       this.player2 = player2;
   }

   public Player play()
   {
       PlayedMove player1Move = player1.chooseMove();
       PlayedMove player2Move = player2.chooseMove();
       if(player1Move == PlayedMove.ROCK && player2Move == PlayedMove.SCISSORS ||
          player1Move == PlayedMove.PAPER && player2Move == PlayedMove.ROCK ||
          player1Move == PlayedMove.SCISSORS && player2Move == PlayedMove.PAPER)
       {
          return player1;
       }
       else if(player2Move == PlayedMove.ROCK && player1Move == PlayedMove.SCISSORS ||
          player2Move == PlayedMove.PAPER && player1Move == PlayedMove.ROCK ||
          player2Move == PlayedMove.SCISSORS && player1Move == PlayedMove.PAPER)
       {
          return player2;
       }
       else
       {
          return null;
       }
   } 
}

Note: I am mostly a C# programmer, hence this code might not contain the most fancy Java features.

Answer 4

In my opinion, the logic contained in the GameLogic#isPlayerWinner method can be exploded into a switch to declutter the logic; this will remove the need to compare the player's move with the computer's move.

Enhanced switch statement - Java 14

private static boolean isPlayerWinner(PlayedMove player, PlayedMove computer) {
   return switch (player) {
      case ROCK -> computer == PlayedMove.SCISSORS;
      case PAPER -> computer == PlayedMove.ROCK;
      case SCISSORS -> computer == PlayedMove.PAPER;
      default -> throw new IllegalArgumentException();
   };
}

Java under 14

private static boolean isPlayerWinner(PlayedMove player, PlayedMove computer) {
   switch (player) {
      case ROCK:
         return computer == PlayedMove.SCISSORS;
      case PAPER:
         return computer == PlayedMove.ROCK;
      case SCISSORS:
         return computer == PlayedMove.PAPER;
      default:
         throw new IllegalArgumentException();
   }
}

Answer 5

The code is readable, and beyond the Winer typo, and the use of brackes-on-different-lines (which I realize is not universal; but that Java code tends to adhere to), it certainly passes muster.

However, I suggest making heavier use of enum (which I find are generally under-utilized) as follows:

enum Move {
    ROCK, 
    SCISSORS,
    PAPER; 
    
    private Move winsAgainst;
    static {
        ROCK.winsAgainst = SCISSORS;
        SCISSORS.winsAgainst = PAPER;
        PAPER.winsAgainst = ROCK;
    }

    public Result against(Move other) {
        if (this == other) return Result.DRAW;

        return (winsAgainst == other) ? Result.WIN : Result.LOSS;
    }
}

A simpler variant using constructors unfortunately does not work due to forward references -- see this question on SO.

I would also rename PlayedMove to Move (it will probably be an inner class, so that the outer class will provide enough context, as in RockPaperScissors.Move), and Winner to Result, so that you can build different games that do not necessarily have a human and a computer competing -- this allows Move to be used unchanged in more contexts.

Answer 6

I was taken with tucuxi's version, but wanted to get more data-driven, with tables to represent combinations of plays, and their outcomes. To take it a stage further, I included the Lizard and Spock ;-)

From the point of view of reviewing the original code, I dislike complex multi-part "if" statements, and will look for clearer ways to represent the logic. In this case, the set of rules can be encapsulated as data (thanks to Roland for suggesting the rule() method), and, for me, that is a preferable approach. Expressing Rock,Paper,Scissors,Lizard,Spock in the original's style would be very ugly...

package com.acme.rps;

import java.util.HashMap;
import java.util.Map;

/**
 * OOP approach to Rock, Paper, Scissors, Lizard, Spock
 */
public class RockPaperScissors {

  private static enum Outcome {
    WIN, LOSE, DRAW;
  }

  private static class Result {

    private Outcome outcome;
    private String winDescription;

    Result(Outcome outcome, String winDescription) {
      this.outcome = outcome;
      this.winDescription = winDescription;
    }

    Outcome getOutcome() {
      return outcome;
    }

    String getWinDescription() {
      return winDescription;
    }
  }

  private static enum Play {

    /* The five "Plays" */
    ROCK, PAPER, SCISSORS, LIZARD, SPOCK;

    /*
     * For the current play,
     * the plays we can defeat and how we defeat them
     */
    Map<Play /*defeatablePlay*/, String /*how*/> winDescriptions = new HashMap<>();

    Result resultAgainst(Play opponentsPlay) {
      if (opponentsPlay == this) { // enums can be compared with object equality
        return new Result(Outcome.DRAW, null);
      }

      // Can we defeat them?
      String winDescription = winDescriptions.get(opponentsPlay);
      if (winDescription != null) {
        return new Result(Outcome.WIN, winDescription);
      }

      // If not, then they must be able to defeat us
      winDescription = opponentsPlay.winDescriptions.get(this);
      return new Result(Outcome.LOSE, winDescription);
    }

    /**
     * A nice way to express one of the rules
     * @param winningPlay
     * @param winDescription
     * @param losingPlay
     */
    static void rule(Play winningPlay, String winDescription, Play losingPlay) {
      winningPlay.winDescriptions.put(losingPlay, winDescription);
    }

    static {
      rule(ROCK, "crushes", SCISSORS);
      rule(ROCK, "crushes", LIZARD);

      rule(PAPER, "covers", ROCK);
      rule(PAPER, "disproves", SPOCK);

      rule(SCISSORS, "cuts", PAPER);
      rule(SCISSORS, "decapitates", LIZARD);

      rule(LIZARD, "eats", PAPER);
      rule(LIZARD, "poisons", SPOCK);

      rule(SPOCK, "smashes", SCISSORS);
      rule(SPOCK, "vaporizes", ROCK);
    }
  }

  /**
   * Show all possible plays and their results
   * @param args ignored
   */
  public static void main(String[] args) {
    for (Play yourPlay : Play.values()) {
      for (Play opponentPlay : Play.values()) {
        System.out.format("You play %s, opponent plays %s, you ", yourPlay.name(), opponentPlay.name());
        Result result = yourPlay.resultAgainst(opponentPlay);
        switch (result.getOutcome()) {
          case DRAW :
            System.out.format("DRAW%n");
            break;
          case LOSE :
            System.out.format("LOSE (%s %s %s)%n", opponentPlay.name(), result.getWinDescription(), yourPlay.name());
            break;
          case WIN :
            System.out.format("WIN (%s %s %s)%n", yourPlay.name(), result.getWinDescription(), opponentPlay.name());
            break;
          default :
            break;
        }
      }
      System.out.println();
    }

  }

}

Answer 7

The main point of OO is to provide structures that allow you to easily extend and modify your application without having to alter existing source code. Having everything in static methods prevent this completely.

How would you extend your code to handle "rock, paper, scissors, lizard, spock"? First the moves have to be taken out of the enum, because the enum prevents extensions, and replace it with an interface: Move.

The game logic is essentially comparing the moves. A Comparator<T extends Move> comes to min but it doesn't cut it because the relations are cyclical and that breaks Comparator's contract. You need to name it MoveEvaluator<T extends Move>.

Players need to know which moves the game supports. A MoveFactory<T extends Move> is used to list the moves.

You probably want to have games decided on best of three or more rounds, because after all, this is a game of skill. So the moves played by the two players are are stored in a Round<T extends Move>. The fate of earth is decided in RPS game against aliens so better not call the players humans or computers, just use player1 and player2.

A Game<T extends Move> holds the number of rounds needed to win the game and the rounds that have been played.

And so on...

I don't blame you if you now want to go code in Python and make memes about Java coders writing endless factories.

Now that you got this far, RPS only works for choosing a winner from two participants. You also have to think about how you would write your code so that it can be extended to allow three or more people?

Answer 8

One important point I would like to add (along with what is suggested earlier). Your class is having only static methods. If class is having only static methods, then:

1. This class should be marked final to prevent inheritance.

2. Class constructor should be private to prevent class initialization.