Spring service for playing Rock Paper Scissors I had created a service which does the actual play of Rock Paper Scissors. 

Below is the service which takes a PlayRequest and updates the State of Player objects given by the PlayerService. 

So the consumer of the API knows about winner, loser or tie by doing a GET on a Player. 

One of the things I feel wrong NOW is that the method play takes a PlayerRequest but uses it only for two lines, all other work is done from the injected dependencies.

Would appreciate any comments related to State as well as SOLID principles.

@Service
public class GameplayService {

  private PlayerService playerService;

  private GameSessionService gameSessionService;

  public GameplayService(GameSessionService gameSessionService, PlayerService playerService) {
    this.gameSessionService = gameSessionService;
    this.playerService = playerService;
  }

  public void play(PlayRequest playRequest) throws RPSException {
    GameSession currentSession = gameSessionService.sessions().get(playRequest.getInviteCode());
    Player player = playerService.changePlayerState(playRequest.getPlayerName(), State.PLAYING);
    currentSession.changeStateTo(GameSession.State.PLAYING);
    try {
      Turn turn = new Turn(player, Enum.valueOf(Move.class, playRequest.getMove()));
      if (currentSession.rounds().isEmpty()) {
        createNewRound(turn, currentSession);
      } else if (!currentSession.rounds().isEmpty()) {
        Round latestRound = currentSession.latestRound();
        if (OVER.equals(latestRound.getState())) {
          createNewRound(turn, currentSession);
        } else if (PLAYING.equals(latestRound.getState())) {
          latestRound.pushLatestTurn(turn);
        }
      }
      Round latestRoundAfterAllUpdates = currentSession.latestRound();
      Optional&lt;Result&gt; resultOptional = latestRoundAfterAllUpdates.getResult();
      if (resultOptional.isPresent()) {
        currentSession.changeStateTo(GameSession.State.WAITING);
        Result result = resultOptional.get();
        if (result.isTie()) {
          currentSession.setTie(true);
        } else {
          currentSession.setWinner(result.getWinner());
        }
        currentSession.getFirstPlayer().changeStateTo(State.WAITING);
        currentSession.getSecondPlayer().changeStateTo(State.WAITING);
      }
    } catch (InvalidOperationException e) {
      throw new RPSException(e.getMessage());
    }
  }

  private void createNewRound(Turn turn, GameSession gameSession) {
    gameSession.addRound(new Round(turn));
  }

}
SOLID Principles

Single responsibility principle


  A class should have one, and only one, reason to change.


This class should take a PlayerRequest and update the state of the corresponding Players. 

The public void play method does more than that, by changing the state of the current game, creating a new round or pushing the latest turn. It would be easier to read the code if the play method was just updating the states and no more. The management of the current Round and Turn should be delegated to a different dependency. Since the Responsibility of the Service class is to update the states, it would be acceptable to have another method wait changing the states to State.WAITING.

Creating unit tests for this class would be then much easier, as we would need only to test the expected change of states and everything else would be mocked.

Open-Closed Principle


  You should be able to extend a class’s behavior, without modifying it.


This block of code, even if moved to another class, could violate the Open-Closed Principle:

      Turn turn = new Turn(player, Enum.valueOf(Move.class, playRequest.getMove()));
      if (currentSession.rounds().isEmpty()) {
        createNewRound(turn, currentSession);
      } else if (!currentSession.rounds().isEmpty()) {
        Round latestRound = currentSession.latestRound();
        if (OVER.equals(latestRound.getState())) {
          createNewRound(turn, currentSession);
        } else if (PLAYING.equals(latestRound.getState())) {
          latestRound.pushLatestTurn(turn);
        }
      }


It is not clear from the code if there are more possible Round States than OVER and PLAYING, but if the Game was extended and there was a new State to be handled (e.g. an EXTRA round), this code block would need to be changed to address it. The solution is to depend upon an abstraction to handle the Round management. Concrete classes will then handle the implementation for the different possible Round States.

Liskov substitution principle


  Let \${\displaystyle \phi (x)}\$ be a property provable about objects \${\displaystyle x}\$ of type T. Then \${\displaystyle \phi (y)}\$ should be true for objects \${\displaystyle y}\$ of type S where S is a subtype of T.


Following this principle will be a consequence of the abstraction made for the previous principle. The derived classes will have to not alter the expected behavior of their parent classes. This principle will be ensured by adding the correct assertions (postconditions) in the unit tests.

Interface Segregation Principle


  Make fine grained interfaces that are client specific.


As @RoToRa suggested, we would need to see the injected interfaces. They should be fine grained in order that the clients (derived classes) will only implement the methods they really use.

Dependency Inversion Principle


  Depend on abstractions, not on concretions.


Here it is pretty straight-forward: the injected dependencies of the GameplayService class should be abstractions (i.e. abstract classes or interfaces). Then it would be easy to change the behavior of the class in case the rules of the Game are extended.

General comments

The best way to check if your code is clean and well-written is to cover it by Unit Tests, even better if it is done using TDD. The complexity of the entire class would be then reduced and the feel of adding collaborators and mocking their behavior would come easier. 

Any possible null pointer exceptions, bugs or redundancies in the code would also be avoided during Unit Testing, like this unnecessary if condition of the else branch:

else if (!currentSession.rounds().isEmpty())


I would also use lambdas when working with Optionals and typical Java 8 features, e.g.:

resultOptional.ifPresent(result -&gt; {...})