Spring MVC login controller

Question

I am making a small project in Spring MVC. My login controller looks like this:

@Controller
public class LoginController 
{   
   @Autowired
   private Environment env;

   private static final Logger LOGGER = LoggerFactory.getLogger(LoginController.class);

   private AuthenticationService authenticationService = AuthenticationService.getAuthenticationInstance();

   @RequestMapping(value = "/login.jsp" , method = RequestMethod.GET)
   public ModelAndView userLoginPage(){
    return new ModelAndView("Login");       
   }

   @RequestMapping(value = "/login", method = RequestMethod.POST)
   public ModelAndView userLogin(@ModelAttribute LoginModel loginModel) {
    LOGGER.info("Employee trying to login [{}] ", loginModel.toString());
    ModelAndView loginView = new ModelAndView();
    loginView.addObject("loginModel", loginModel);
    String login = authenticationService.authenticateUser(loginModel,env);
    switch (login) {
    case "Authenticated":
        loginView.setViewName("dashboard");
        break;
    case "Authenticated First time Login":
        loginView.setViewName("changepassword");
        break;  
    case "Not Authenticated":
        loginView.setViewName("Login");
        loginView.addObject("invalid", "invalid user/password");
        break;
    }

    return loginView;
 }

     @RequestMapping(value = "/changePassword" , method = RequestMethod.POST)
     public ModelAndView userPasswordChange(@ModelAttribute 
       PasswordChangeModel passwordChangeModel){
     LOGGER.info("Employee trying to change password [{}] 
       ",passwordChangeModel.getUserid());
     authenticationService.changePassword(passwordChangeModel, env);
     return new ModelAndView("passwordchangesuccess");
   }

 }

I am using switch case to delegate to different views. Is this the right approach or should I do something else to delegate response to views?

Answer 1

The switch has a bad reputation and is usually seen as a code smell.

In your case, I would first propose to replace the strings by one enum so that you have strong typing, you avoid typos errors and your Ide can warn you if you miss a case in your switch. By the way, it is a good practice to have a default case to handle and spot the unexpected cases (I usually throw a UnsupportedOperationException or IllegalArgumentException). You should of course avoid to have some MVC part into your AuthenticationService but that is a job for the adapters and his friends.

If you really want to remove this switch (I wish) then the idea is to use polymorphism. Basically, your AuthenticationService should return a subtype of a basic result class (LoginSuccess, LoginFailed) where the view name will be defined or computed.

About your controller itself, you should use @Autowired for Environment and AuthenticationService. Also note that Spring discourage the usage of autowired fields outside of tests classes.