3
\$\begingroup\$

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;

enter image description here

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Oct 30, 2021 at 19:35

1 Answer 1

2
\$\begingroup\$

I like to work with the request specific annotation. Also, you don't need @RequestMapping, unless you want to version your API with URL versioning or have some fixed path for all your endpoints.

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

    private final GameService gameService;

    private final ObjectMapper mapper;

    @GetMapping
    public String index() {
        return "game";
    }

    @GetMapping("/login")
    public String login() {
        return "login";
    }

    @Secured("ROLE_USER")
    @PostMapping("/games")
    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")
    @PutMapping("/games/{gameId}")
    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);
    }
}

I would create a separate Controller for error handling:

@Controller
public class CustomErrorController implements ErrorController {

    @RequestMapping("/error")
    public String handleError(HttpServletRequest request) {
        Object status = request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
        if (status != null) {
            int statusCode = Integer.parseInt(status.toString());
            if (statusCode == HttpStatus.NOT_FOUND.value()) {
                return "404";
            } else if (statusCode == HttpStatus.INTERNAL_SERVER_ERROR.value()) {
                return "500";
            } else if (statusCode == HttpStatus.FORBIDDEN.value()) {
                return "403";
            }
        }
        return "error";
    }
}

When using Spring Boot you don't even need a controller, because Spring will look for specific error pages (like 403.html) in src/main/resources/templates/error/ before defaulting to the generic error.html page whenever your application encounters an error or exception.

\$\endgroup\$
4
  • \$\begingroup\$ request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE) returns null for 403 forbidden page as request doesn't have a attribute of RequestDispatcher.ERROR_STATUS_CODE. What should I do? \$\endgroup\$
    – Burak
    Oct 31, 2021 at 17:37
  • 2
    \$\begingroup\$ i had a similar review ad advised to separate the errorhandling as well - good proposal! \$\endgroup\$ Nov 1, 2021 at 5:34
  • \$\begingroup\$ @Burak Where is this request originating from? Maybe you can ask a question on Stack Overflow and show your new code. If you link it in a comment I will find it. \$\endgroup\$
    – H3AR7B3A7
    Nov 1, 2021 at 12:37
  • \$\begingroup\$ @H3AR7B3A7 I can edit this question and post the new code. By the way, I solved that problem so you can just review my new version of controller? \$\endgroup\$
    – Burak
    Nov 1, 2021 at 19:48

Your Answer

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

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