4
\$\begingroup\$

I have created a Spring Boot Microservice for playing Rock Paper Scissrors. I have tried to follow best practices, but still would appreciate some critique on my code. Thanks.

@RestController
public class RockPaperScissorsController {

  @Autowired
  private PlayerService playerService;

  @Autowired
  private HttpServletRequest context;

  @Autowired
  private GameSessionService gameSessionService;

  @Autowired
  private GameplayService gameplayService;

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

  @GetMapping(value = "/ping", produces = "application/json")
  public ResponseEntity<String> ping() {
    return ResponseEntity.ok("{\"response\":\"pong\"}");
  }

  @GetMapping(value = "/player/{playerName}", produces = "application/json")
  public ResponseEntity player(@PathVariable("playerName") String playerName) {
    try {
      Player player = playerService.getPlayer(playerName);
      return ResponseEntity.ok(player);
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/player/{playerName}", produces = "application/json")
  public ResponseEntity playerPOST(@PathVariable("playerName") String playerName) {
    try {
      playerService.createPlayer(playerName);
      return ResponseEntity.ok().body("");
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }


  @DeleteMapping(value = "/player/{playerName}", produces = "application/json")
  public ResponseEntity playerDELETE(@PathVariable("playerName") String playerName) {
    try {
      playerService.deletePlayer(playerName);
      return ResponseEntity.ok("");
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/createInvite/{playerName}", produces = "application/json")
  public ResponseEntity createInvite(@PathVariable("playerName") String inviter) {
    try {
      Player player = playerService.getPlayer(inviter);
      GameSession session = gameSessionService.createSessionFrom(new Invite(player));
      return ResponseEntity.ok(session);
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/acceptInvite/{inviteCode}/{playerName}", produces = "application/json")
  public ResponseEntity acceptInvite(@PathVariable("inviteCode") String inviteCode,
      @PathVariable("playerName") String playerName) throws InvalidOperationException {
    try {
      Player player = playerService.getPlayer(playerName);
      return ResponseEntity.ok(gameSessionService.acceptInvite(player, inviteCode));
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @GetMapping(value = "/session/{inviteCode}", produces = "application/json")
  public ResponseEntity session(@PathVariable("inviteCode") String inviteCode) {
    return ResponseEntity.ok(gameSessionService.sessions().get(inviteCode));
  }

  @PostMapping(value = "/readyplayer/{playername}", produces = "application/json")
  public ResponseEntity ready(@PathVariable("playername") String playerName) {
    try {
      Player player = playerService.changePlayerState(playerName, Player.State.READY);
      return ResponseEntity.ok(player);
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/play", produces = "application/json")
  public ResponseEntity play(@RequestBody PlayRequest playRequest) {
    try {
      gameplayService.play(playRequest);
      return ResponseEntity.ok().body("");
    } catch (RPSException e) {
      e.printStackTrace();
    }
    return ResponseEntity.status(HttpStatus.NOT_IMPLEMENTED).body("");
  }

}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
  1. You do not need ping within your controller. Check out Spring boot actuator
  2. Each method having duplicated error handling. You can move it to @ControllerAdvice
  3. Method names like playerPOST, playerDELETE aren't helpful. Consider this as a normal class and have normal method names like createPlayer, deletePlayer
  4. You need not define produces attribute for each mapping, RestController defaults to JSON
  5. Returning ok("") doesn't make sense. Should be either returning the player object, or use ResponseEntity.created() (201)
  6. Why are you autowiring HttpServletRequest context?
  7. Should use AllArgsConstructor - constructor based autowiring
\$\endgroup\$
0
1
\$\begingroup\$
  1. field-based vs constructor-based dependency injection
    Placing the @AutoWired annotation on the instance variables means the Spring container will use field-based injection. It will NOT use the constructor to populate the dependencies. If you want to signal to the container to use constructor-based injection, you need to place the annotation on the method. and why the constructor is initializing only two dependencies when there are three?

  2. ping return
    returning hard-coded json String is not best practice. in the future, you may want to extend the functionality for example to return detailed status of external resources (like Database, storage, etc). either use a Map or a user defined POJO.

  3. Method names
    playerPOST, playerDELETE is both uninformative as well as doesn't follow naming convention. why no use the names of the service methods? and why return an empty String? just don't return any body at all.

  4. producing json
    Spring has an enumerated set of values for produces attribute. class name: org.springframework.http.MediaType

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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