Rest Controller in Spring Boot for Rock Paper Scissors REST API

Question

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

Answer 1

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

Answer 2

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