I have a Spring MVC controller but I'm not sure that it is a good or bad design. As far as I know, api versioning is missing but apart from that I implemented Swagger for documentation and added SpringSecurity and tried to follow YARAS(Yet Another RESTful API Standard) to build it but I need another eye on that to comment it.

@Slf4j
@Controller
@RequestMapping
@RequiredArgsConstructor
public class XGameController implements GameController {

    private final GameService gameService;

    private final ObjectMapper mapper;

    @RequestMapping(value = "/", method= RequestMethod.GET)
    public String index() {
        return "game";
    }

    @RequestMapping(value = "/login", method= RequestMethod.GET)
    public String login() {
        return "login";
    }

    @Secured("ROLE_USER")
    @RequestMapping(value = "/games", method= RequestMethod.POST)
    public String initializeGame(Model model) {
        log.info("New XGame is initializing...");
        Game game = new Game();
        game = gameService.initializeGame(game.getId());

        try {
            model.addAttribute("game", mapper.writeValueAsString(game));
        } catch (JsonProcessingException e) {
            log.error(e.getMessage());
        }
        log.info("New XGame is initialized successfully!");
        return "game";
    }

    @Secured("ROLE_USER")
    @RequestMapping(value = "/games/{gameId}", method= RequestMethod.PUT)
    public @ResponseBody Game play(@PathVariable("gameId") String gameId,
                                   @RequestParam Integer pitNumber,
                                   @RequestParam String action) {
        log.info("Sowing stone is triggered...");
        return gameService.executeGameRules(UUID.fromString(gameId), pitNumber);
    }

    @RequestMapping(value = "/403", method= RequestMethod.GET)
    public String error403() {
        return "/error/403";
    }

}

My swagger snapshot;

CodePudding user response：

I would make some changes.

1. In /games/{gameId} I would use PATCH instead of PUT. The reason is that PUT is intended to completely replace the resource (in your case, the Game). This does not seem to be what you are doing in this endpoint. PATCH is intended to partially update a resource, which seems much more suited to what you are doing here.

2. Still in /games/{gameId} I would use the request body to provide the needed data instead of query parameters. It simply doesn't seem right. Query parameters are way more suited to GET requests than to POST, PUT or PATCH.

3. I would rename /403 to something else that actually gives some context about what 403 is. Having said this, I would go with /error-pages/403. Additionally, I would also consider removing this endpoint from the swagger specification.

Other than this, it seems fine to me.

CodePudding user response：

1. Firstly, instead of @RequestMapping use a specific Mapping(Get, Post,etc.) and the use of type of mapping is up to you which you find more particular to the cause of using it

2. if you are redirecting from a page to homepage try to use return "redirect:"/url"" instead of just returning HTML file directly.

3. Rename your method for error, RequestMapping value to some more reasonable name.

4. instead of using return "/error/403" use return "redirect:/error/403"