Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 id;
    private String title;
    private String description;

    public Game(){ }

    public Game(int id, String title, String description) {
        this.id = id;
        this.title = title;
        this.description = description;
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    public String getTitle() {
        return title;
    }

    public void setTitle(String title) {
        this.title = title;
    }

    public String getDescription() {
        return description;
    }

    public void setDescription(String description) {
        this.description = description;
    }   
}

GameRepository

public class GameRepository implements IGameRepository{
    private List<Game> games = new ArrayList<>();

    public GameRepository(){
        games.add(new Game(1, "League of Legends", "fun game")); 
        games.add(new Game(5, "Call of Duty 4", "awesome game")); 
        games.add(new Game(4, "Farmville", "gtfo")); 
    }

    @Override
    public List<Game> getGames() {
        return games;
    }

    @Override
    public Game getGameById(int id) {
        for(Game game : games){
            if(game.getId() == id){
                return game;
            }
        }

        throw new IllegalArgumentException("This id is not found");
    }

    @Override
    public void updateGame(Game game) {
        Iterator<Game> it = games.iterator();
        while(it.hasNext()){
            Game currentGame = it.next();

            if(currentGame.getId() == game.getId()){
                it.remove();
                break;
            }
        }

        games.add(game);
    }
}

IGameRepository

public interface IGameRepository {
    public List<Game> getGames();
    public Game getGameById(int id);
    public void updateGame(Game game);
}

GameValidator

public class GameValidator implements Validator {
    @Override
    public boolean supports(Class<?> type) {
        return type == Game.class;
    }

    /* Arbitrary restrictions on the description:
        - Between 5 and 25 characters
        - Cannot contain a number
    */
    @Override
    public void validate(Object o, Errors errors) {
        Game game = (Game) o;

        String description = game.getDescription();
        if(description.length() < 5 || description.length() > 25){
            errors.rejectValue("description", "TODO:localization", "Stay within the boundaries");
        }

        if(description.matches(".*\\d.*")){
            errors.rejectValue("description", "TODO:localization", "NO NUMBERS!");
        }
    }
}

ApplicationContext

<bean id="gameRepository" class="domain.GameRepository" /> 
<bean id="gameValidator" class="domain.GameValidator" />

Index.jsp

<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title>Overview</title>
    </head>

    <spring:url value="/games/details/" var="detailsUrl" />
    <spring:url value="/games/edit/" var="editUrl" />

    <body>
        <h2>Games</h2>
        <table>
            <thead>
                <th> Id </th>
                <th> Title </th>
                <th> Description </th>
            </thead>
            <tbody>
                <c:forEach items="${games}" var="game" varStatus="status">
                    <tr>
                        <td>${game.id}</td>
                        <td>${game.title}</td>
                        <td>${game.description}</td>
                        <td><a href="${detailsUrl}${game.id}.htm">Details</a></td>
                        <td><a href="${editUrl}${game.id}.htm">Edit</a></td>
                    </tr>
                </c:forEach>
            </tbody>
        </table>
    </body>
</html>

Details.jsp

<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title>Details - ${currentGame.title}</title>
    </head>
    <body>
        <h1> Details for ${currentGame.title} </h1>

        <p>
            Description: ${currentGame.description}
        </p>
    </body>
</html>

Edit.jsp

<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title>Edit - ${currentGame.title}</title>
    </head>

    <body>
        <style>
            .error { color: red; }
        </style>

        <h1>Update game information</h1>

        <form:form method="POST" modelAttribute="edit_game_form">
            <form:errors path="*" cssClass="error" element="div"/>
            <form:hidden path="title" />
            <table>
                <tr>
                    <td>
                        Description:
                    </td>
                    <td>
                        <form:input path="description" />
                    </td>
                    <td>
                        <form:errors path="description" cssClass="error" />
                    </td>
                </tr>

                <tr>
                    <td colspan="3">
                        <input type="submit" value="Update" />
                    </td>
                </tr>
            </table>
        </form:form>
    </body>
</html>
share|improve this question

2 Answers 2

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) {
share|improve this answer
    
What would the Map be mapping exactly? Those setters aren't needed indeed, I just did a quick generate code in netbeans. The empty constructor is needed for Spring, something to do when I submit the form. I assume it's used to construct a new object and then fill its fields with reflection. –  Jeroen Vannevel Aug 26 at 20:38
    
Map of id -> Game. I'm not familiar with spring-web, but "regular" spring dependency injection supports initialization with contructor-args not only with setters. I prefer using immutable fields and initializing in constructor, as that way you know that the constructed object is complete. –  janos Aug 26 at 20:43
    
The Map would be Map<Integer, Game> and I was just thinking the same thing about that –  Simon André Forsberg Aug 26 at 20:44
    
Ah, like that. It doesn't feel very right to make the link between id and Game so loose. It's an interesting approach though, I'll keep it in mind. I see what you mean with the DI. I'm not sure if that's an option but I'll take a look at it later. –  Jeroen Vannevel Aug 26 at 20:47
2  
That link already exists in your code. Using a Map would make it easier to see for readers, making your code more intuitive, and at the same time more efficient. In any case, the mapping is in the private implementation of the class, so it's a hidden internal detail. I don't see anything wrong with it. –  janos Aug 26 at 20:52

I think you shouldn't declare your beans in the XML configuration if you can eeasily avoid it. Just annotate GameRepository and GameValidator with @Repository and @Component respectively, just like you have annotated GameController with @Controller. (See about the different annotations.)

Then, I definitely second janos's comment about using a Map. Much more readable and conventional.

Next, you don't need to declare methods as public in IGameRepository. That's the only kind of a method an interface can define, so it's redundant. No harm done, but anyway. I find it a bit more clear when you leave the extra keywords out.

Finally, I don't know what topics your exam covers, but I'd definitely practice Spring Security and try creating a login form and different users so that some could both edit and view the games, whereas others could only view them. And learn the details about a Spring bean life cycle.

share|improve this answer
    
I was under the impression that the XML beans were a big plus for spring because it allows you to change behaviour on the fly. Is this incorrect? I'll take the other remarks with me for future purposes (for feedback: the exam happened to be extremely similar to this exercise I whipped up so that went awesome). –  Jeroen Vannevel Aug 27 at 20:50
    
I don't think there's much that you can do with XML that you couldn't do with annotations, and since you were already using annotations, I'd find it consistent if you used annotations all the way. That being said, I believe they both have their uses, but it doesn't look like this particular case would benefit from an XML configuration in any way. The differences have been discussed a lot at StackOverflow -- for example: stackoverflow.com/questions/15698795 stackoverflow.com/questions/8428439 stackoverflow.com/questions/182393 –  ZeroOne Aug 28 at 9:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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