Skip to main content
added 55 characters in body
Source Link

@Autowired is not required on constructor in Spring over 4.3 (class has to be annotated as bean and have only one constructor).

Inject SimpMessagingTemplate template in a constructor. Now you mix annotation and constructor injection.

IMHO you shouldn't put any business login in controller methods. I would move it all to separate service. For example I would write updatePerson() like this:

@MessageMapping("/person/update")
public void updatePerson(@Payload Person requestPerson) {
    personService.updatePerson(requestPerson);
    this.template.convertAndSend("/topic/person", personService.viewPersons());
}

In updatePerson() and createPerson() you should have separate objects instead of Person class. E.g. CreatePersonRequest and UpdatePersonRequest

I would change viewPersons() to getAllPersons()

The rest look fine :)

@Autowired is not required on constructor in Spring over 4.3 (class has to be annotated as bean and have only one constructor).

Inject SimpMessagingTemplate template in a constructor. Now you mix annotation and constructor injection.

IMHO you shouldn't put any business login in controller methods. I would move it all to separate service. For example I would write updatePerson() like this:

@MessageMapping("/person/update")
public void updatePerson(@Payload Person requestPerson) {
    personService.updatePerson(requestPerson);
    this.template.convertAndSend("/topic/person", personService.viewPersons());
}

In updatePerson() and createPerson() you should have separate objects instead of Person class. E.g. CreatePersonRequest and UpdatePersonRequest

The rest look fine :)

@Autowired is not required on constructor in Spring over 4.3 (class has to be annotated as bean and have only one constructor).

Inject SimpMessagingTemplate template in a constructor. Now you mix annotation and constructor injection.

IMHO you shouldn't put any business login in controller methods. I would move it all to separate service. For example I would write updatePerson() like this:

@MessageMapping("/person/update")
public void updatePerson(@Payload Person requestPerson) {
    personService.updatePerson(requestPerson);
    this.template.convertAndSend("/topic/person", personService.viewPersons());
}

In updatePerson() and createPerson() you should have separate objects instead of Person class. E.g. CreatePersonRequest and UpdatePersonRequest

I would change viewPersons() to getAllPersons()

The rest look fine :)

Source Link

@Autowired is not required on constructor in Spring over 4.3 (class has to be annotated as bean and have only one constructor).

Inject SimpMessagingTemplate template in a constructor. Now you mix annotation and constructor injection.

IMHO you shouldn't put any business login in controller methods. I would move it all to separate service. For example I would write updatePerson() like this:

@MessageMapping("/person/update")
public void updatePerson(@Payload Person requestPerson) {
    personService.updatePerson(requestPerson);
    this.template.convertAndSend("/topic/person", personService.viewPersons());
}

In updatePerson() and createPerson() you should have separate objects instead of Person class. E.g. CreatePersonRequest and UpdatePersonRequest

The rest look fine :)