11
\$\begingroup\$

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));
    }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ I think your solution depends on the need for the "problem". That is why you should include a statement that details the use case (s) of the game itself. For example, what happens if you win 3 times in a row. In this way we can know how well your solution fits the problem itself. \$\endgroup\$ – Andres Gardiol Nov 25 '20 at 15:11
  • \$\begingroup\$ I suggest that you expand the game to Rock Paper Scissors Spock Lizard, see samkass.com/theories/RPSSL.html. That way you have the same logic, but have more cases to cover. If the code gets messy when doing this you probably should look into other ways to solve the problem. \$\endgroup\$ – Polygorial Nov 27 '20 at 9:50
10
\$\begingroup\$

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..
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Thanks for the advice, I appreciate it. Regarding code formatting, that is not part of Java conventions. There are many generally accepted code styles. In Eclipse IDE, and probably in many other IDEs, you can define your own code auto format. \$\endgroup\$ – Zoran Jankov Nov 25 '20 at 19:30
  • 12
    \$\begingroup\$ The java convention has always been that curly braces go on the same line. oracle.com/technetwork/java/codeconventions-150003.pdf \$\endgroup\$ – Eric Stein Nov 26 '20 at 2:15
  • 1
    \$\begingroup\$ Oracle's convention document published in 1997 is not evidence of "always", for several reasons: Java existed before that; styles have changed since then; not everybody agrees with Oracle anyway. Braces on the same line is a popular style today, but by no means the only style used. now or in the past. \$\endgroup\$ – amalloy Nov 26 '20 at 3:13
  • 9
    \$\begingroup\$ @amalloy braces on the same line is used in every single open source Java library / framework I know of. Its pretty much the de-facto standard of the ecosystem, even if there are some exceptions. That is notably different in C#, where the prevalent style is braces on new lines, which is done by MS and widely adopted in the C# ecosystem. "When in rome, do as the romans do" is always a good advice to follow. \$\endgroup\$ – Polygnome Nov 26 '20 at 8:57
  • \$\begingroup\$ People tend to use the oracle/sun conventions, and java has the conventions specified. Its not the only style in use by any means, but its not something that I would have brought up. I feel its more comment worthy than a point I'd bring up in a code review of an independant library. But java style is the one mentioned here. \$\endgroup\$ – Athas Nov 26 '20 at 9:02
14
\$\begingroup\$

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).

\$\endgroup\$
4
  • \$\begingroup\$ Yes, I now see that I could transfer the code of the GameLogic class in the Winner enum. \$\endgroup\$ – Zoran Jankov Nov 25 '20 at 19:24
  • 1
    \$\begingroup\$ @ZoranJankov Winner is useful for many contexts; I would put the logic to see which Move wins against which inside the Move enum \$\endgroup\$ – tucuxi Nov 26 '20 at 10:09
  • 2
    \$\begingroup\$ @tucuxi I would handle the logic neither in the Winner nor in the Move enums. Having a separate function/method is totally fine. \$\endgroup\$ – Roland Illig Nov 29 '20 at 1:54
  • 1
    \$\begingroup\$ @RolandIllig in OOP, placing that function/method inside a well-chosen class is also fine :-) \$\endgroup\$ – tucuxi Nov 29 '20 at 12:34
5
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ You could offload move-outcome computation int the move, and make your Game class ignore details of PlayerMove \$\endgroup\$ – tucuxi Nov 26 '20 at 9:54
  • \$\begingroup\$ I also think that the comparison part of this answer is not the best. For this, I recommend the other answers. With this answer, I wanted to show how one could make OP's code more object-oriented by introducing classes and an interface for the players. \$\endgroup\$ – SomeBody Nov 27 '20 at 14:44
4
\$\begingroup\$

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();
   }
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
11
  • \$\begingroup\$ The game logic should not be programmed into a class that describes the action as that prevents one from extending the game logic. The action should only know about itself. Game logic should be placed in a class whose responsibility is to handle game logic. \$\endgroup\$ – TorbenPutkonen Nov 26 '20 at 10:35
  • 5
    \$\begingroup\$ @TorbenPutkonen we disagree. The fact that rock wins vs scissors is very coupled to those values, and placing it in a different class adds little clarity. Mixing logic with data when closely coupled is a common OO practice. If other games need to be implemented, different OtherGame.Move enums, possibly with a common interface, would be my choice. \$\endgroup\$ – tucuxi Nov 26 '20 at 11:10
  • 1
    \$\begingroup\$ You can use object equality rather than .equals() methods... Otherwise, the code is exemplary, in my view, as it's more data-driven than procedural. \$\endgroup\$ – Mark Bluemel Nov 26 '20 at 11:32
  • \$\begingroup\$ @tucuxi I have a real grudge against placing logic inside enums. My view is that it always violates single responsibility principle. It extends the enum contract in non-standard ways and always surprises the maintainer. This particular example is a highly specialized use case but with a more generic enum it makes the code unreusable. \$\endgroup\$ – TorbenPutkonen Nov 27 '20 at 7:33
  • 1
    \$\begingroup\$ @TorbenPutkonen, I vehemently disagree. It doesn't violate SRP, since any logic should be tightly coupled to the values. Uncle bob himself uses and recommends this in Clean Code. As to maintainability, placing logic elsewhere is way worse: I added a new value, now where are all the places I need to change that should handle this new value? Also, the same unfamiliar code example (as strained as it is) can be used the other way as well: I have a move, what can I do with it? Ctrl+F PlayerMoveUtils? \$\endgroup\$ – Celos Nov 27 '20 at 10:12
3
\$\begingroup\$

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();
    }

  }

}
\$\endgroup\$
2
  • \$\begingroup\$ Could you perhaps add a little icing on the cake, in that the rules read rule(ROCK, "crushes", LIZARD)? This would make them even easier to read and verify. \$\endgroup\$ – Roland Illig Nov 29 '20 at 20:30
  • 1
    \$\begingroup\$ Thanks @RolandIllig - I've incorporated your rule() method suggestion, which I think is a nice touch. \$\endgroup\$ – Mark Bluemel Nov 30 '20 at 12:05
2
\$\begingroup\$

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?

\$\endgroup\$
2
  • 4
    \$\begingroup\$ Building an over-general application is a case of premature optimization. What about multi-player? Player ELO calculation? Scalability and network failure? Instead of over-designing from the start, I advocate writing readable code that is easy to refactor if and when requirements change. OPs' program is, in that regard, better than what you propose. \$\endgroup\$ – tucuxi Nov 26 '20 at 11:18
  • \$\begingroup\$ @tucuxi You're absolutely correct in that. But for the sake of the requested OOP aspects, I stand by my answer. :) I mean, we're striving to making better code in general, not just the best possible throwaway hack. \$\endgroup\$ – TorbenPutkonen Nov 27 '20 at 7:40
1
\$\begingroup\$

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.

\$\endgroup\$

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.