Spring service for playing Rock Paper Scissors

Question

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

• RoToRa: At the very least you should post the interfaces of the other classes you are using. Better would be runnable code, e.g. with unit tests that mock the parts you haven't posted.

Answer 1

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

• GauravEdekar: Thanks a lot for all your generous comments. You are welcome to review more code if you get the chance to: github.com/gavarava/rockpaperscissors

Answer 2

To Your Comment

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.

For me it feels right if the statement playerService.changePlayerState(playRequest.getPlayerName(), State.PLAYING) would be inside currentSession.changeStateTo(GameSession.State.PLAYING).

I would replace the statement by something like:

gameSessionService.enterPlayingState(currentSession);

The reasons behind this feeling are:

• we loose the dependency to playerService in GameplayService
• we loose the dependency to State.PLAYING
• we reduce the number of lines and some complexity

To My Comments

Sadly you provide only a little code snipped of your application so it is hard to give you good advice without knowing what the other components do.. but here is my try :)

Reduce Complexity

At the first glance it is not possible to see what the method play do. This has multiple reasons:

• has 3 levels of nesting
• is longer than it has to be

It is possible to divide play in 3 sections:

• enter the playing state
• play the turn
• evaluate the round

public void play(PlayRequest playRequest) throws >RPSException {
 // enter playing state
 GameSession currentSession = gameSessionService.sessions().get(playRequest.getInviteCode());
 /* ... */
 try {
 // play turn
 Turn turn = new Turn(player, Enum.valueOf(Move.class, playRequest.getMove()));
 /* ... */
 // evaluate round
 Round latestRoundAfterAllUpdates = currentSession.latestRound();
 /* ... */
 } catch (InvalidOperationException e) {
 throw new RPSException(e.getMessage());
 }
}

Law of Demeter

The expression gameSessionService.sessions().get(playRequest.getInviteCode()) violates the Law of Demeter.

Only talk to your immediate friends.

GameSessionService is a intermediate friend of GameplayService but with get(playRequest.getInviteCode()) we "talk" to the friend of our friend.

The statement could be also expressed as gameSessionService.prodiveBy(playRequest).

The following statements violates the Law of Demeter too:

currentSession.rounds().isEmpty()
currentSession.getFirstPlayer().changeStateTo(State.WAITING);
currentSession.getSecondPlayer().changeStateTo(State.WAITING);

Feature Envy

A method accesses the data of another object more than its own data.

When we look at

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<Result> resultOptional = latestRoundAfterAllUpdates.getResult();
/* ... */
private void createNewRound(Turn turn, GameSession gameSession) {
 gameSession.addRound(new Round(turn));
}

We use the variable currentSession 6 times in this little code snipped with the goal to update it by deciding how to handle the update by its data which violates the feature envy.

We should simply call a method on currentSession:

Result result = currentSession.play(player, playRequest.getMove());

Polymorphism and If-Else-Statements

Optional<Result> resultOptional = latestRoundAfterAllUpdates.getResult();
/* ... */
Result result = resultOptional.get();
if (result.isTie()) {
 currentSession.setTie(true);
} else {
 currentSession.setWinner(result.getWinner());
}

With an Optional we can express in Java that there could be no result after a method call. For example if we search in a database for a id.

But in this case we try to express different typs of Result:

• NoResult (the optional is empty case)
• Tie
• Win

The goal would be to enter the next state of the game by the result which could be expressed as

currentState.enterNextStateBy(result)

The goal

The goal of a refactoring would be to have only some method calls which show the flow a play takes

Form your the little snipped you provide it could look like:

public void play(PlayRequest playRequest) throws >RPSException {
 GameSession session = gameSessionService.prodiveBy(playRequest);
 session.enterPlayState();
 Result result = session.play(player, playRequest.getMove());
 currentState.enterNextStateBy(result);
}

Last Advice

I would take a look into the State pattern to handle the states of your game.

• GauravEdekar: Thanks a lot for all your generous comments. You are welcome to review more code if you get the chance to: github.com/gavarava/rockpaperscissors