I've got a spring controller class which handles the user logins.

I've got two endpoints:

  1. /user/facebook
  2. /user/email

You can probably guess what these do, they each create an account (user or facebook). But they do use some of the same logic, which I'm wondering, is it worth trying to make a re-usable class out of it?

I'm pretty new to java so I'm probably not making use of it's full potential!

The setup I've got going is we've got a user entity which has multiple user credentials...This means a user with email X can login with multiple different account types. E.g If I logged in with FB and then logged in with the same email that's attached to the facebook account, it'd merge them (after an email is sent for verification). I've added a lot of comments in my code for you to read.

package com.userapi.spring.controller;
public class UserController {
    UserCredentialService userCredService; // Just a DAO

    ConfirmHandler handler;

    UserService userService; // Just a DAO

    JwtTokenServiceImpl jwtServ;

    // Used to grab data from our redis store...
    RedisClient redisClient = new RedisClient();

    // #TODO should these be up here or should I create a new UserCredential in each of the endpoints
    UserCredential userCredential = null;

    @RequestMapping(value="login/facebook", method=RequestMethod.POST,  produces = {MediaType.APPLICATION_JSON_VALUE })
    ResponseEntity<?> login(@RequestBody UserFacebookLoginContext body) throws IOException, InvalidKeySpecException,
            NoSuchAlgorithmException, ConfirmLogin, ClassNotFoundException, ConfirmResponse, InvalidRequestException, ConfirmLoginEmail {

        long fbId;
        String fbToken = body.getFacebookToken();
        String email = null;
        if (fbToken.equals("") || fbToken.length() < 10) throw new Error("Invalid facebook token"); // Quick check on fb token 
        // Create new user entity (auth_user)
        String hashPassword = userService.hashPassword("hashxx--29");

        // Make call to graph and check if data is returned
        FacebookClient facebookClient = new DefaultFacebookClient(fbToken, Version.LATEST);
        com.restfb.types.User userData = facebookClient.fetchObject("me",
                com.restfb.types.User.class, Parameter.with("fields", "id,email,hometown,cover,name,picture"));

        // Check if token was valid, grab email + id
        if (userData.getEmail() != null
                && userData.getHometown() != null && userData.getId() != null) {
            fbId = Long.parseLong(userData.getId());
            email = userData.getEmail();
        } else {

            throw new InvalidRequestException("Invalid facebook login");
//            throw new Error("Invalid facebook login"); // TODO add generic error class which spits back approriate msgs
        // Simply serializes the returned facebook graph data into a Java obj
        FacebookDataSerializer fbUser = new FacebookDataSerializer(userData);

        // Create JSON string from fbdata
        ObjectMapper mapper = new ObjectMapper();
        String fbJson = mapper.writeValueAsString(fbUser); // We save this in a DB field (just dumping it for now)

        // Find existing user credentials for this email #TODO change findBYExternalId so it returns a user credential not list
        List<UserCredential> foundUserEmail = userCredService.findByEmail(userData.getEmail());
        // Filter out found users by externalId
        List<UserCredential> foundUsers = foundUserEmail.stream().filter(u -> u.getExternalId() == fbId)

        // If a credential exists, check if it matches the parsed email + passsowrd + account type -
        if (foundUserEmail.size() > 0 && foundUsers.size() == 0) {
            // Create new user credential #TODO make re-usable with the below user credential....
            userCredential = new UserCredential(
                    new Date(),
            // This sends an email to the user with a link that they must click to merge the two accounts
            ConfirmHandler handler = new ConfirmHandler(userCredential);
            handler.storeConfirmationObject(); // Store in redis...

            // Return a custom message object in respinse entiy
            ConfirmResponse r = new ConfirmResponse("Error", 111, "An account already exists under this email",
                    "Please goto your email (" + userCredential.getEmail() + ") and confirm to login!",
                    "You must goto your email, click the 'create account' button, and then you'll be redirected to our site!");

        } else if (foundUsers.size() > 0) {
            System.out.println("\nFb user is valid...return  foundUsers.credential" + email);
            userCredential = foundUsers.get(0);
            if (!userCredential.getEmail().equals(email))  throw new Error("Something strange going on"); // Just incase something weird is going on...
        } else {
            // Replaces setters with constructor (#TODO should I use setters instead)??
            User user = new User(
                    0,  // is admin user
                    email, // Username (storing email for now)
                    1, // is normal user
                    0, // is staff user
                    new Date(),
                    new Date()
            // Create new user credential (should I use getters or constructor)
            userCredential = new UserCredential(
                    user.getUserId(), // Grab the userId from the found or created user entity
                    new Date(),
            // Save user credential to database

        // Create new JWT
        JwtToken jwt = new JwtToken();
        // Create JWT for usercredential (userId) and simply return 
        return new ResponseEntity<>(new FbLoginResponse(jwt.getToken(), fbUser), HttpStatus.OK); // Returns fb graph data + JWT


    @RequestMapping(value="/api/v1/logout/", method=RequestMethod.POST,  produces = {MediaType.APPLICATION_JSON_VALUE })
    LogoutResponse deleteToken(final HttpServletRequest request) throws ServletException {
        final Claims claims = (Claims) request.getAttribute("claims");
        System.out.println(request.getAttribute("token")+ " <~~~ About to logout user");

        boolean didLogout = redisClient.delete(request.getAttribute("token").toString());
        return new LogoutResponse("Logged out", !didLogout);

    @RequestMapping(value="login/email", method=RequestMethod.POST,  produces = {MediaType.APPLICATION_JSON_VALUE })
    LoginResponse login(@RequestBody UserEmailLoginContext body) throws InvalidKeySpecException, NoSuchAlgorithmException, ConfirmLogin, IOException, ClassNotFoundException {
        String email = body.getEmail();
        String password = body.getPassword();

        if (email.equals(""))  throw new Error("No email provided");
        else if (password.equals(""))throw new Error("No password provided");
        String hashPassword = userService.hashPassword(password);
        List<UserCredential> foundUsers = userCredService.findByEmail(email); // #TODO move to just single usercredential

        // If a credential exists, check if it matches the passed email + password + account type
        if (foundUsers.size() > 0) {
            userCredential = userCredService.isValidUserLogin(foundUsers, email, password);
            if (userCredential == null) {
                // Determine whether there are other credential types in the isValidUserLogin method
                // instead of throwing an error to end the program
                throw new Error("Invalid user");
        } else {
            // Create new user entity (auth_user)
            User user = new User();
            user.setDateJoined(new Date());

            // Create new user credential with the passed email + password
            userCredential = new UserCredential();
            userCredential.setCreated(new Date());

        JwtToken jwt = new JwtToken();

        return new LoginResponse(jwt.getToken());

As you can see there is a lot of duplicated logic, I'm not sure how I'd split it up, or IF I should?

Edit 1:

I've replaced those long constructors with setters...I feel like it's more readable and you know exactly what data is being set.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.