Today, I’m going to show you how I have performed refactor process on some piece of code. There is an app – ITSM Tool demo – helpdesk ticket handling system – it’s not ideally coded, that’s why it’s a good example for refactoring. The app is a ‘classic’ CRUD in Spring Boot w/ Thymeleaf and MSSQL on db side. The thing is that I will refactor it to get rid off the bad code as well as apply CQS – command query separation. You can find the code on my github: https://github.com/hipacy/itsm-tool
In the second step I will rewrite its architecture from regular ‘common model’ with repository/service/model layers to the one described in the CQRS approach.
What I’d like to achieve?
I like to present how I can refactor code by applying good practices and showing how to do such cleaning. In the next I will apply the command query separation.
The refactoring is quite easy and might seem obvious for more experienced of you guys – so if you want more interesting stuff – go straight to the CQRS part.
Refactor phase – what’s wrong with the code?
Firstly let’s have a look at the project structure, it goes like this:
It’s a ‘classic’ layered structure. I have rolled out packages which I’d like to focus on. Before I get to CQRS transformation, I’d like to clean up those classes first.
Controllers
TicketController:
/// import statements
@Controller
public class TicketController {
private static final String TICKET_MODEL_NAME = "ticket";
private static final String SUCCESS_MESSAGE_MODEL_NAME = "successMessage";
@Autowired
private UserService userService;
@Autowired
private TicketService ticketService;
@Autowired
private TeamService teamService;
@Autowired
private MessageService messageService;
@GetMapping(value = "/createTicket")
public ModelAndView createTicket() {
ModelAndView modelAndView = new ModelAndView();
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
modelAndView.addObject("edit", false);
modelAndView.addObject(TICKET_MODEL_NAME, new Ticket());
modelAndView.addObject("user", user);
modelAndView.setViewName("ticketEditor");
return modelAndView;
}
@PostMapping(value = "/createTicket")
public ModelAndView createTicket(@Valid Ticket ticket, BindingResult bindingResult, RedirectAttributes rr) {
ModelAndView modelAndView = new ModelAndView();
if (bindingResult.hasErrors()) {
rr.addFlashAttribute("errorMessage", "Fill in all fields!!!");
rr.addFlashAttribute("edit", false);
modelAndView.setViewName("redirect:/createTicket");
} else if (ticket != null && ticket.getTicketBody() != null && ticket.getTitle() != null) {
ticket.setRequestorId(userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName()));
ticket.setCreationDate(LocalDateTime.now());
ticket.setLastModifiedDate(LocalDateTime.now());
ticket.setStateId(new StateOfTicket(1, "New"));
ticket.setPrioId(new Prio(1, "Low"));
ticketService.saveTicket(ticket);
rr.addFlashAttribute("edit", false);
rr.addFlashAttribute(SUCCESS_MESSAGE_MODEL_NAME, "Ticket has been added!");
modelAndView.setViewName("redirect:/createTicket");
}
return modelAndView;
}
@GetMapping(value = "/editTicket-{id}")
public ModelAndView editTicket(@PathVariable Integer id) {
ModelAndView modelAndView = new ModelAndView();
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
Ticket ticket = ticketService.findTicketById(id);
modelAndView.addObject("user", user);
modelAndView.addObject(TICKET_MODEL_NAME, ticket);
if (userService.checkUser(id, ticket, modelAndView, user)) return modelAndView;
modelAndView.addObject(TICKET_MODEL_NAME, ticketService.findTicketById(id));
modelAndView.addObject("edit", true);
modelAndView.setViewName("ticketEditor");
return modelAndView;
}
@PostMapping(value = "/editTicket-{id}")
public ModelAndView editTicket(@PathVariable Integer id, @Valid Ticket ticket, BindingResult bindingResult,
RedirectAttributes rr) {
ModelAndView modelAndView = new ModelAndView();
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
if (bindingResult.hasErrors()) {
rr.addFlashAttribute("edit", true);
rr.addFlashAttribute("errorMessage", "Fill in all fields!!!!");
modelAndView.setViewName("redirect:/editTicket-" + id);
} else if (ticket != null) {
rr.addFlashAttribute("edit", true);
ticket.setRequestorId(user);
ticket.setLastModifiedDate(LocalDateTime.now());
Ticket currentTicket = ticketService.findTicketById(id);
currentTicket.setTicket(ticket);
ticketService.saveTicket(currentTicket);
rr.addFlashAttribute(SUCCESS_MESSAGE_MODEL_NAME, "Ticket has been edited!");
modelAndView.setViewName("redirect:/editTicket-" + id);
}
return modelAndView;
}
@PostMapping(value = "/sendMessage-{id}")
public ModelAndView addMessage(ModelAndView modelAndView, @PathVariable Integer id,
Message message, RedirectAttributes rr) {
if (ticketService.checkId(modelAndView, id)) return modelAndView;
Ticket currentTicket = ticketService.findTicketById(id);
message.setCreator(userService.findUserByEmail(
SecurityContextHolder.getContext().getAuthentication().getName()));
message.setDateTimeCreated(LocalDateTime.now());
message.setTicketId(currentTicket);
if (!currentTicket.getRequestorId().getRoles().contains(new UserType(2, "USER"))) {
message.setIsTechnical(true);
} else {
message.setIsTechnical(false);
}
messageService.saveMessage(message);
rr.addFlashAttribute(SUCCESS_MESSAGE_MODEL_NAME, "Message sent!");
modelAndView.setViewName("redirect:/ticketView-" + id);
return modelAndView;
}
@PostMapping(value = "/ticketView-{id}")
public ModelAndView ticketView(ModelAndView modelAndView, @PathVariable Integer id,
Ticket ticket, RedirectAttributes rr) {
if (ticketService.checkId(modelAndView, id)) return modelAndView;
Ticket currentTicket = ticketService.findTicketById(id);
currentTicket.setLastModifiedDate(LocalDateTime.now());
currentTicket.setTicket(ticket);
ticketService.saveTicket(currentTicket);
rr.addFlashAttribute(SUCCESS_MESSAGE_MODEL_NAME, "Ticket has been edited!");
modelAndView.setViewName("redirect:/ticketView-" + id);
return modelAndView;
}
@GetMapping(value = "/ticketView-{id}")
public ModelAndView ticketViewEdit(@PathVariable Integer id) {
ModelAndView modelAndView = new ModelAndView();
if (id != null) {
Ticket currentTicket = ticketService.findTicketById(id);
if (userService.checkUser(id, currentTicket, modelAndView, userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName())))
return modelAndView;
List<Team> ticketTeams = teamService.findAllTicketTeams(id);
if (!ticketTeams.isEmpty()) {
Team team = ticketTeams.get(ticketTeams.size() - 1);
modelAndView.addObject("team", team);
}
modelAndView.addObject(TICKET_MODEL_NAME, currentTicket);
modelAndView.addObject("messages", messageService.findMessagesByTicketId(currentTicket));
modelAndView.addObject("message", new Message());
modelAndView.addObject("technicians", userService.findUserByRoles(
new UserType(3, "TECHNICIAN")));
modelAndView.addObject("softwareItems", currentTicket.getItems().stream().filter(
it -> it.getType().equals("Software")).collect(Collectors.toList()));
modelAndView.addObject("hardwareItems", currentTicket.getItems().stream().filter(
it -> it.getType().equals("Hardware")).collect(Collectors.toList()));
modelAndView.addObject("otherItems", currentTicket.getItems().stream().filter(
it -> it.getType().equals("Other")).collect(Collectors.toList()));
modelAndView.setViewName("ticketView");
} else {
modelAndView.setViewName("access-denied");
return modelAndView;
}
return modelAndView;
}
}
Issues:
- mixed responsibilities for methods editTicket, addMessage, ticketView, createTicket – they take care as well as for application logic – changes state of objects and interacts with the views (which it should be doing).
- there’s also too much string literals used repeatedly.
- unnecessary conditional statements (redundant nullchecks)
Fixes:
- move code responsible for mutating objects (tickets, messages) to the service layers.
- create separate classes for static string literals.
Let’s examin createTicket methods – here we have two overloaded ones first handles GET and the other one :
@GetMapping(value = "/createTicket")
public ModelAndView createTicket() {
ModelAndView modelAndView = new ModelAndView();
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
modelAndView.addObject("edit", false);
modelAndView.addObject(TICKET_MODEL_NAME, new Ticket());
modelAndView.addObject("user", user);
modelAndView.setViewName("ticketEditor");
return modelAndView;
}
@PostMapping(value = "/createTicket")
public ModelAndView createTicket(@Valid Ticket ticket, BindingResult bindingResult, RedirectAttributes rr) {
ModelAndView modelAndView = new ModelAndView();
if (bindingResult.hasErrors()) {
rr.addFlashAttribute("errorMessage", "Fill in all fields!!!");
rr.addFlashAttribute("edit", false);
modelAndView.setViewName("redirect:/createTicket");
} else if (ticket != null && ticket.getTicketBody() != null && ticket.getTitle() != null) {
ticket.setRequestorId(userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName()));
ticket.setCreationDate(LocalDateTime.now());
ticket.setLastModifiedDate(LocalDateTime.now());
ticket.setStateId(new StateOfTicket(1, "New"));
ticket.setPrioId(new Prio(1, "Low"));
ticketService.saveTicket(ticket);
rr.addFlashAttribute("edit", false);
rr.addFlashAttribute(SUCCESS_MESSAGE_MODEL_NAME, "Ticket has been added!");
modelAndView.setViewName("redirect:/createTicket");
}
return modelAndView;
}
Firstly, #1, I would take care of the ModelAndView declaration. It can be moved to the parameter of the method (that’s what Spring let’s us doing). I’m going to do that for all methods in this class.
#2, the conditional in the ‘post’ method is not needed there – the @Valid annotation by the Ticket parameter does that for me – it validates whether the field is null or not. It is set in the entity’s params (@NotNull annotation): https://github.com/hipacy/itsm-tool/blob/master/src/main/java/com/rocketzki/itsmtooldemo/model/Ticket.java
#3, I need to move the object constructing section (setting ticket’s properties) to the service layer – a controller is not a good place for that kind of operations.
4#, I will move all string literals used as parameters for model and template names to separate classes (like modelAndView.setViewName(“redirect:/createTicket”) or modelAndView.addObject(“user”, user))
5#, The odd authenticatin method checkUser will be replaced by spring security annotation.
That’s how it looks like after my changes:
@Secured({"ADMIN", "TECHNICIAN"})
@GetMapping(value = "/createTicket")
public ModelAndView createTicket(ModelAndView modelAndView) {
modelAndView.addObject(ModelNames.EDIT_FLAG_MODEL_NAME, false);
modelAndView.addObject(ModelNames.TICKET_MODEL_NAME, new Ticket());
modelAndView.addObject(ModelNames.USER_MODEL_NAME,
userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName()));
modelAndView.setViewName(TemplateNames.TICKET_EDITOR_TEMPLATE_NAME);
return modelAndView;
}
@Secured({"ADMIN", "TECHNICIAN"})
@PostMapping(value = "/createTicket")
public ModelAndView createTicket(ModelAndView modelAndView,
@Valid Ticket ticket,
BindingResult bindingResult,
RedirectAttributes rr) {
rr.addFlashAttribute(ModelNames.EDIT_FLAG_MODEL_NAME, false);
if (bindingResult.hasErrors()) {
rr.addFlashAttribute(ModelNames.ERROR_MESSAGE_MODEL_NAME, "Fill in all fields!!!");
modelAndView.setViewName(TemplateNames.REDIRECT_PREFIX + TemplateNames.CREATE_TICKET_TEMPLATE_NAME);
} else {
ticketService.createTicket(ticket, userService.findUserByEmail(SecurityContextHolder
.getContext().getAuthentication().getName()));
rr.addFlashAttribute(ModelNames.SUCCESS_MESSAGE_MODEL_NAME, "Ticket has been added!");
modelAndView.setViewName(TemplateNames.REDIRECT_PREFIX + TemplateNames.CREATE_TICKET_TEMPLATE_NAME);
}
return modelAndView;
}
The code looks is more readable and responsibilities have been separated as they should. I have created a createTicket method in service layer instead of setting model properties within the controller. Then I’ve swapped string litterals with static variables and created separate classes for that matter.
Changes made in the service layer look like this when it comes to ticket creation:
public void createTicket(Ticket ticketData, Users user) {
Optional.ofNullable(ticketData).ifPresent(ticket ->
saveTicket(TicketMapper.INSTANCE.mapNewTicket(ticket, user))
);
}
All the logic responsible for mapping values is moved to the mapper. Then the service calls saveTicket which invokes repository and saves changes to the database. Notice that I could change the accessibility of the saveTicket method to private.
private Ticket saveTicket(Ticket ticket) {
return ticketRepository.save(ticket);
}
Mapper, does the nullcheck and sets the properties – its is used by TicketService for ticket editing and creation:
@UtilityClass
public class TicketMapper {
public static void mapTicket(Ticket current, Ticket edited, Users user) {
if (current != null && edited != null) {
mapTicket(current, edited);
current.setRequestorId(user);
}
}
public static Ticket mapNewTicket(Ticket current, Users user) {
if (current != null) {
current.setRequestorId(user);
current.setCreationDate(LocalDateTime.now());
current.setStateId(new StateOfTicket(1, "New"));
return current;
} else {
return null;
}
}
public static void mapTicket(Ticket current, Ticket edited) {
if (edited.getPrioId() != null) {
current.setPrioId(edited.getPrioId());
}
List<Item> list = edited.getItems();
if (list != null) {
current.setItems(new ArrayList(list));
}
List<Team> list1 = edited.getAssignedTeams();
if (list1 != null) {
current.setAssignedTeams(new ArrayList(list1));
}
if (edited.getCategory() != null) {
current.setCategory(edited.getCategory());
}
if (edited.getAssignedTechnicianId() != null) {
current.setAssignedTechnicianId(edited.getAssignedTechnicianId());
}
if (edited.getTitle() != null) {
current.setTitle(edited.getTitle());
}
if (edited.getTicketBody() != null) {
current.setTicketBody(edited.getTicketBody());
}
if (edited.getStateId() != null) {
current.setStateId(edited.getStateId());
}
if (edited.getApprovalNeeded() != null) {
current.setApprovalNeeded(edited.getApprovalNeeded());
}
if (edited.getRequestorId() != null) {
current.setRequestorId(edited.getRequestorId());
}
if (edited.getCreationDate() != null) {
current.setCreationDate(edited.getCreationDate());
}
current.setLastModifiedDate(LocalDateTime.now());
}
}
As I mentioned earlier – a logic responisble for object mutation (changing its state) has been moved to separate class – a mapper.
Ok so that’s how I’ve refactored createTicket methods. Let’s have a look at the remaining ones: editTicket and ticketView
@GetMapping(value = "/editTicket-{id}")
public ModelAndView editTicket(@PathVariable Integer id) {
ModelAndView modelAndView = new ModelAndView();
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
Ticket ticket = ticketService.findTicketById(id);
modelAndView.addObject("user", user);
modelAndView.addObject(TICKET_MODEL_NAME, ticket);
if (userService.checkUser(id, ticket, modelAndView, user)) return modelAndView;
modelAndView.addObject(TICKET_MODEL_NAME, ticketService.findTicketById(id));
modelAndView.addObject("edit", true);
modelAndView.setViewName("ticketEditor");
return modelAndView;
}
@PostMapping(value = "/editTicket-{id}")
public ModelAndView editTicket(@PathVariable Integer id, @Valid Ticket ticket, BindingResult bindingResult,
RedirectAttributes rr) {
ModelAndView modelAndView = new ModelAndView();
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
if (bindingResult.hasErrors()) {
rr.addFlashAttribute("edit", true);
rr.addFlashAttribute("errorMessage", "Fill in all fields!!!!");
modelAndView.setViewName("redirect:/editTicket-" + id);
} else if (ticket != null) {
rr.addFlashAttribute("edit", true);
ticket.setRequestorId(user);
ticket.setLastModifiedDate(LocalDateTime.now());
Ticket currentTicket = ticketService.findTicketById(id);
currentTicket.setTicket(ticket);
ticketService.saveTicket(currentTicket);
rr.addFlashAttribute(SUCCESS_MESSAGE_MODEL_NAME, "Ticket has been edited!");
modelAndView.setViewName("redirect:/editTicket-" + id);
}
return modelAndView;
}
Issues: the same as createTicket methods had plus: – checkUser method which checks user’s privileges for accessing certain websites if (userService.checkUser(id, ticket, modelAndView, user)) return modelAndView; – there’s a better solution. Reduntant null check for ticket (it’s validated already) else if (ticket != null). The last thing is that ticket properties are set within the controller – that should be delegated elsewehere either.
Fixes: apply the same changes as for createTicket methods, replace checkUser method with Spring Security annotation. Use already created static string values.
Effect:
@Secured({"ADMIN", "TECHNICIAN"})
@GetMapping(value = "/editTicket-{id}")
public ModelAndView editTicket(ModelAndView modelAndView,
@PathVariable Integer id) {
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
Ticket ticket = ticketService.findTicketById(id);
modelAndView.addObject(ModelNames.USER_MODEL_NAME, user);
modelAndView.addObject(ModelNames.TICKET_MODEL_NAME, ticket);
modelAndView.addObject(ModelNames.EDIT_FLAG_MODEL_NAME, true);
modelAndView.setViewName(TemplateNames.TICKET_EDITOR_TEMPLATE_NAME);
return modelAndView;
}
@Secured({"ADMIN", "TECHNICIAN"})
@PostMapping(value = "/editTicket-{id}")
public ModelAndView editTicket(ModelAndView modelAndView,
@PathVariable Integer id,
@Valid Ticket ticket,
BindingResult bindingResult,
RedirectAttributes rr) {
Users user = userService.findUserByEmail(SecurityContextHolder.getContext().getAuthentication().getName());
if (bindingResult.hasErrors()) {
rr.addFlashAttribute(ModelNames.ERROR_MESSAGE_MODEL_NAME, "Fill in all fields!!!!");
} else {
ticketService.editTicket(ticket, user, id);
rr.addFlashAttribute(ModelNames.SUCCESS_MESSAGE_MODEL_NAME, "Ticket has been edited!");
}
rr.addFlashAttribute(ModelNames.EDIT_FLAG_MODEL_NAME, true);
modelAndView.setViewName(TemplateNames.REDIRECT_PREFIX + "editTicket-" + id);
return modelAndView;
}
Code is more clear and easier to read. Look at the logic responsible for ticket object mutation (setters). It’s not here 🙂 It has been moved to the service which delegates it to the mapper. Also the editing method has been secured by Spring Security annotation.
I have created two convenience methods for ticket editing – it results from the ‘business’ logic of the application:
public void editTicket(Ticket editedTicketData, Users user, Integer id) {
Optional.ofNullable(editedTicketData).ifPresent(ticket -> {
Ticket currentTicket = findTicketById(id);
TicketMapper.mapTicket(currentTicket, ticket, user);
saveTicket(currentTicket);
});
}
public void editTicket(Ticket editedTicketData, Integer id) {
Optional.ofNullable(editedTicketData).ifPresent(ticket -> {
Ticket currentTicket = findTicketById(id);
TicketMapper.mapTicket(currentTicket, ticket);
saveTicket(currentTicket);
});
}
Just like in the previous example – the saving responsibility has been moved to separate method and the saveTicket method is not directly called from the controller.
The last two methods (ticketView) have been changed accordingly.
Ther is one last one thing I want to mention regarding TicketController – I have moved sendMessage method (responsible for adding messages to tickets) to a separate class – MessageController:
// imports
@Controller
public class MessageController {
@Autowired
private UserService userService;
@Autowired
private TicketService ticketService;
@Autowired
private MessageService messageService;
@PostMapping(value = "/sendMessage-{id}")
public ModelAndView addMessage(ModelAndView modelAndView,
@PathVariable Integer id,
Message message,
RedirectAttributes rr) {
if (ticketService.checkId(modelAndView, id)) return modelAndView;
Ticket currentTicket = ticketService.findTicketById(id);
messageService.createNewMessage(message, userService.findUserByEmail(
SecurityContextHolder.getContext().getAuthentication().getName()), currentTicket);
rr.addFlashAttribute(ModelNames.SUCCESS_MESSAGE_MODEL_NAME, "Message sent!");
modelAndView.setViewName(TemplateNames.REDIRECT_PREFIX + TemplateNames.TICKET_VIEW_TEMPLATE_NAME + "-" + id);
return modelAndView;
}
}
So that’s how I have refactored module responsible for ticket handling. Let’s summarize what has been done:
- separated logic responsible for object mutation from service and controller layers
- created mappers which handle the above
- removed redundant methods replacing them with those provided by spring framework
- replaced string literals with static variables moved to separate classes
- removed redundant null checks and conditional statements.
The same steps have to be done to the UserController. The class is less messy but it needs some tidying up too. It suffers from exactly the same issues as its predecessor: string literals (minor issue) and mixed responsibilities (object mutation – a major issue) – look how I got rid off the block of setters for currentUser variable.
One important thing: I have moved method responsible for user registration to the UserController method – it suits more in there when it comes to the domain – it handles user administration logic.
Old class:
//imports
@Controller
public class UserController {
private static final String PROFILE_TEMPLATE_NAME = "profile";
@Autowired
private UserService userService;
@GetMapping(value = {"/profile"})
public ModelAndView getUser(ModelAndView mav) {
mav.addObject("user", userService.findUserByEmail(
SecurityContextHolder.getContext().getAuthentication().getName()));
mav.addObject("edit", false);
mav.setViewName(PROFILE_TEMPLATE_NAME);
return mav;
}
@GetMapping(value = {"/profile-{id}"})
public ModelAndView getUserById(ModelAndView mav, @PathVariable Integer id) {
mav.addObject("user", userService.findUsersByUserId(id));
mav.addObject("edit", false);
mav.setViewName(PROFILE_TEMPLATE_NAME);
return mav;
}
@GetMapping(value = {"/editUser-{id}"})
public ModelAndView editUser(ModelAndView mav, @PathVariable Integer id) {
mav.addObject("user", userService.findUsersByUserId(id));
mav.addObject("edit", true);
mav.setViewName(PROFILE_TEMPLATE_NAME);
return mav;
}
@PostMapping(value = {"/editUser-{id}"})
public ModelAndView doEditUser(ModelAndView mav,
@PathVariable Integer id,
Users user,
BindingResult br, RedirectAttributes rr) {
Users currentUser = userService.findUsersByUserId(id);
currentUser.setJobTitle(user.getJobTitle());
currentUser.setFirstName(user.getFirstName());
currentUser.setLastName(user.getLastName());
currentUser.setEmail(user.getEmail());
userService.updateUser(currentUser);
if (!br.hasErrors()) {
rr.addFlashAttribute("successMessage", "User edited successfully!");
} else {
rr.addFlashAttribute("errorMessage", "User couldn't be edited, try again.");
}
rr.addFlashAttribute("edit", false);
mav.setViewName("redirect:/profile-" + id);
return mav;
}
@GetMapping(value = {"/usersList"})
public ModelAndView getAllUsers(ModelAndView mav) {
List<Users> users = userService.findAll();
mav.addObject("allUsers", users);
mav.setViewName("userList");
return mav;
}
}
Cleaned class with added responsibilities from LoginController:
@Controller
public class UserController {
@Autowired
private UserService userService;
@Autowired
private TeamService teamService;
@GetMapping(value = {"/profile"})
public ModelAndView getUser(ModelAndView modelAndView) {
modelAndView.addObject(ModelNames.USER_MODEL_NAME, userService.findUserByEmail(
SecurityContextHolder.getContext().getAuthentication().getName()));
modelAndView.addObject(ModelNames.EDIT_MODEL_NAME, false);
modelAndView.setViewName(TemplateNames.PROFILE_TEMPLATE_NAME);
return modelAndView;
}
@GetMapping(value = {"/profile-{id}"})
public ModelAndView getUserById(ModelAndView modelAndView, @PathVariable Integer id) {
modelAndView.addObject(ModelNames.USER_MODEL_NAME, userService.findUsersByUserId(id));
modelAndView.addObject(ModelNames.EDIT_MODEL_NAME, false);
modelAndView.setViewName(TemplateNames.PROFILE_TEMPLATE_NAME);
return modelAndView;
}
@GetMapping(value = {"/editUser-{id}"})
public ModelAndView editUser(ModelAndView modelAndView, @PathVariable Integer id) {
modelAndView.addObject(ModelNames.USER_MODEL_NAME, userService.findUsersByUserId(id));
modelAndView.addObject(ModelNames.EDIT_MODEL_NAME, true);
modelAndView.setViewName(TemplateNames.PROFILE_TEMPLATE_NAME);
return modelAndView;
}
@PostMapping(value = {"/editUser-{id}"})
public ModelAndView editUser(ModelAndView modelAndView,
@PathVariable Integer id,
Users user,
BindingResult br, RedirectAttributes rr) {
userService.updateUser(user, id);
if (!br.hasErrors()) {
rr.addFlashAttribute(ModelNames.SUCCESS_MESSAGE_MODEL_NAME, "User edited successfully!");
} else {
rr.addFlashAttribute(ModelNames.ERROR_MESSAGE_MODEL_NAME, "User couldn't be edited, try again.");
}
rr.addFlashAttribute(ModelNames.EDIT_MODEL_NAME, false);
modelAndView.setViewName(TemplateNames.REDIRECT_PREFIX + TemplateNames.PROFILE_TEMPLATE_NAME + "-" + id);
return modelAndView;
}
@GetMapping(value = {"/usersList"})
public ModelAndView getAllUsers(ModelAndView modelAndView) {
modelAndView.addObject(ModelNames.ALL_USERS_MODEL_NAME, userService.findAll());
modelAndView.setViewName(TemplateNames.USER_LIST_TEMPLATE_NAME);
return modelAndView;
}
@GetMapping("/registration")
public ModelAndView registration(ModelAndView modelAndView) {
modelAndView.addObject(ModelNames.EDIT_MODEL_NAME, false);
modelAndView.addObject(ModelNames.AVAILABLE_TEAMS_MODEL_NAME, teamService.findAllTeams());
modelAndView.addObject(ModelNames.USER_MODEL_NAME, new Users());
modelAndView.setViewName(TemplateNames.REGISTRATION_TEMPLATE_NAME);
return modelAndView;
}
@PostMapping(value = "/registration")
public ModelAndView createNewUser(ModelAndView modelAndView, @Valid Users user, BindingResult bindingResult) {
modelAndView.addObject(ModelNames.ALL_USERS_MODEL_NAME, teamService.findAllTeams());
modelAndView.addObject(ModelNames.EDIT_MODEL_NAME, false);
Optional.ofNullable(userService.findUserByEmail(user.getEmail())).ifPresent(
u -> bindingResult.rejectValue("email", "error.user", "The email address is in use already!"));
if (bindingResult.hasErrors()) {
modelAndView.addObject(ModelNames.ERROR_MESSAGE_MODEL_NAME, "Fill in all fields!!");
modelAndView.setViewName(TemplateNames.REGISTRATION_TEMPLATE_NAME);
} else {
userService.saveNewUser(user);
modelAndView.addObject(ModelNames.SUCCESS_MESSAGE_MODEL_NAME, "User has been registered!");
modelAndView.addObject(ModelNames.USER_MODEL_NAME, new Users());
modelAndView.setViewName(TemplateNames.REGISTRATION_TEMPLATE_NAME);
}
return modelAndView;
}
The biggest change was done in the editUser post method. I removed the block of setters invocation and replaced it by single update method moved to the UserService which uses mapper for that operation:
public Users updateUser(Users userData, Users currentUser) {
UserMapper.mapEditedUser(userData, currentUser);
return usersRepository.save(currentUser);
}
public Users updateUser(Users userData, Integer currentUserId) {
Users currentUser = usersRepository.findUsersByUserId(currentUserId);
UserMapper.mapEditedUser(userData, currentUser);
return usersRepository.save(currentUser);
}
Those two methods above can be used for updating user data.
The mapper:
@UtilityClass
public class UserMapper {
public static void mapEditedUser(@NotNull Users userData, @NotNull Users currentUser) {
currentUser.setJobTitle(userData.getJobTitle());
currentUser.setFirstName(userData.getFirstName());
currentUser.setLastName(userData.getLastName());
currentUser.setEmail(userData.getEmail());
}
}
That’s it, when it comes to the refactor of controllers and services. It will make the way to implement CQRS. See you in my next articles! Code can be found here: https://github.com/hipacy/itsm-tool
0
0