HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaspringMinor

Super sophisticated game information web application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
applicationsupersophisticatedgamewebinformation

Problem

Considering I've got an exam on Spring tomorrow, I figured you could review a small example application I whipped together as preparation (and as such, I'd prefer the emphasis to be on Spring-related concepts).

Its functionality is straightforward: you start with a list of 3 games and you can edit or view its details, both which will lead to a different page. The former will allow you to update its description while also providing validation on the field.

Any suggestions?

GameController

@Controller
@RequestMapping("/games")
public class GameController {
@Autowired
private IGameRepository gameRepository;

@Autowired
private GameValidator gameValidator;

@RequestMapping(value = "/all", method = RequestMethod.GET)
public String index(Model model){
model.addAttribute("games", gameRepository.getGames());
return "index";
}

@RequestMapping(value = "/details/{id}", method = RequestMethod.GET)
public String details(Model model, @PathVariable("id") int id){
Game game = gameRepository.getGameById(id);
model.addAttribute("currentGame", game);
return "details";
}

@RequestMapping(value = "/edit/{id}", method = RequestMethod.GET)
public String edit(Model model, @PathVariable("id") int id){
Game game = gameRepository.getGameById(id);
model.addAttribute("edit_game_form", game);
return "edit";
}

@RequestMapping(value = "/edit/{id}", method = RequestMethod.POST)
public ModelAndView update(@ModelAttribute("edit_game_form") @Valid Game game, BindingResult bindingResult, Model model){
gameRepository.updateGame(game);

gameValidator.validate(game, bindingResult);
if(bindingResult.hasErrors()){
return new ModelAndView("edit", "edit_game_form", game);
}

model.addAttribute("games", gameRepository.getGames());
return new ModelAndView("index");
}
}


Game

`public class Game {
private int i

Solution

Game

In the code you shared, you never use the setters of the Game class. How about making its fields final and removing the setters? Immutable things are simple, robust, and great. It's best to make everything immutable as much as you can.

Also, do you really need the empty constructor?

GameRepository

In getGameById and updateGame you iterate over a list of games. How about using a Map of id -> Game instead? It would seem a lot more natural, and at the same time more efficient.

Formatting

Overall the code is well readable. The one thing where you violate the standard is spaces around braces and parentheses:

for(Game game : games){
    if(game.getId() == id){


The standard (Eclipse does it with Control-Shift-f) would be:

for (Game game : games) {
    if (game.getId() == id) {

Code Snippets

for(Game game : games){
    if(game.getId() == id){
for (Game game : games) {
    if (game.getId() == id) {

Context

StackExchange Code Review Q#61117, answer score: 7

Revisions (0)

No revisions yet.