where to put methods that manipulate objects

Question

I have a controller method as follow:

public class RoomsController {

@RequestMapping(method = RequestMethod.GET, path="/v1/rooms/{name}")
    public ResponseEntity<?> getRoomInformation(@PathVariable String name, @RequestHeader("Auth-Token") String token){
        try {
            Room room = roomService.findByLogin(name);
            User user = userService.findByBackendToken(token);

            if(room == null || InstantHelper.biggerThanSixHours(room.getUpdatedAt()))
                room = gitHubService.createOrUpdateRoom(name, user.getAccessToken());

            String roomJson = new RoomFormatter(room).toJson();
            joinRoom(room.getLogin(), token);

            return new ResponseEntity<String>(roomJson, HttpStatus.OK);
        } catch (ChathubBackendException e) {
            log.error(e);
            return new ResponseEntity<>("Error: "+e.getMessage(), HttpStatus.NOT_FOUND);
        }
    }

as seen, I have this line, for example: roomService.findByLogin(name); but from what I understand, a Service does not return any value, it's just execute some action and raise an exception if something goes wrong. This is the first thing.
The second is related to the formatter/join part

String roomJson = new RoomFormatter(room).toJson();
joinRoom(room.getLogin(), token);
return new ResponseEntity<String>(roomJson, HttpStatus.OK);

I'm not really sure if the controller should handle this amount of responsibility like know if it's time for update the room information from API, explicit call the RoomFormatter. I'm kinda lost here, because I don't really know where to put this. I think I should have a in-between layer that is not a service nor a repository, and this layer should know what to do with the room, format the JSON, and so on, but I don't know what it should be or if there is a pattern for this kind of things. Maybe it's ok to have this things in the controller...anyway. Any ideas are welcome!

Answer 1

Design questions like these often tend to have no true or false answer. It depends heavily on the surrounding components and the bigger targets. Despite of this, here some things seem rather clear to me.

So to your first question:

This:

Room room = something.findByLogin(name);

is a task that clearly belongs to the model layer. You can imagine testing this method by a unit test suite that deals only with model classes.

Here, I'd like to add two things. First, as your context is Spring, it is also a question how your model is made accessible by the application container. In the Java world, there are beans. Could it be that by writing "Service", somebody (or you) is confusing the "application container's object live cycle service" and the "service layer of the application itself"?

Second, in short you have a service for the following advantages: 1) Distinguishing the necessary web type activity, 2) managing transactions (also for nested services or multiple models), 3) adding some automated post or pre action steps. Assuming this, a service is rather something bigger than a controller, so you are right about the return values. It is often a good idea to nest a controller into a service.

In my personal opinion, I'd like to see this:

Room room = rooms.findByLogin(name);
User user = users.findByBackendToken(token);

Plus, I would expect to have rooms and users delivered as beans by AOP tags or by the application controller XML config. With users, I was especially careful as this is about security, authentication and authorization, so I would expect to look out for a security framework that allows me access to users.

Then to your second question:

You are right with the nagging feeling about the amount of work a controller has to do. Remind some basic rules how to write good functions: They should do only one clearly named thing. They should give either some value in return, or they should have a side effect, but not both. Of course those rules are being disputed, but they are most often a good guide. They are helping to stick to the principle of least astonishment (or, the smallest amount of WTF per minute).

So if I was a new programmer in your team and if I would read:

getRoomInformation(); 

then yes, I could be surprised if the room (or room list?) occasionally was reformatted. But this doesn't absolutely mean you have to redesign the whole thing. If this is a rare case, and adding other general structures is not justified, then you could make more clarity just by a proper, speaking naming. For instance, imagine this:

@RequestMapping(method = RequestMethod.GET, path="/v1/rooms/{name}")
    public ResponseEntity<?> getRoomInformationAndUpdateXY(@PathVariable String name, @RequestHeader("Auth-Token") String token){
        try {
         ...

Well, let's turn the blind eye. Then it's ok for once. It is ok because the reader doesn't stumble, but she/he is being alerted. Even better is to outsource the reformatting part as an own method, thus keeping the controller clean.

There could be more powerful concepts for actions pre and post the controller, if you need them. Maybe a wrapping service layer, as mentioned in the first part, is the best solution. Or with Spring, you can use interceptors: https://stackoverflow.com/questions/9212699/does-spring-mvc-have-the-concept-of-before-after-controller-action-events Maybe this is the best solution. Or it is an overhead. Or it is not working in your situation. I cannot know, but your decision starts with considerations like this.