API method to validate Facebook OAuth token

Question

I am new to Java / OOP and I'm concerned that I have a method which is doing far too much "stuff" - but I don't easily see how it can be shortened in a way which is not contrived / arbitrary.

This is a server-side API method built using Google App Engine.

Currently, the method (which is really just a template at this stage):

1. Validates a user's Facebook OAuth token against Facebook's own servers - this is all done behind the scenes in a FacebookHelper class I have written.
2. Catches any exceptions (most likely an IOException) thrown during this validation process.
3. Asks the user to log in again if their token is invalid (if (authToken == null)).
4. Else if authentication has been successful, tries to get the user record from my database.
5. If it doesn't exist, adds the user to the database. (registerUserInDatabase).
6. Responds with a personalised welcome to the user - after having registered the user if they were not already registered.

Code:

@ApiMethod(name = "getUserData", path = "get_user_data")
public Bean getUserData(@Named("token") String token) {

    Bean response = new Bean();
    FacebookAuthToken authToken;
    User user;
    String userPersonalisedWelcome;

    try {
        authToken = ServerFacebookHelper.getAuthToken(token);
    } catch (Exception e) {
        response.setData("Exception occurred");
        return response;
    }

    if (authToken == null) {
        response.setData("Token invalid, please log in");
        return response;
    } else {
        user = getUserFromDatabase(authToken);
        if (user == null) {
            user = registerUserInDatabase(authToken);
            userPersonalisedWelcome = user.getPersonalisedWelcome;
            response.setData(userPersonalisedWelcome);
        } else {
            response.setData(user.getPersonalisedWelcome);
        }
        return response;
    }
}

Obviously there is already a huge amount of work being done outside this method, such as API calls and database reads/writes, but it still feels far too procedural and frankly "dumb". At the same time, everything it encapsulates is required in order to service this "getUserData" API request - so perhaps I am overthinking here.

Answer 1

You have a lot of null checks (well, two at least), which I think leads to less readable code. Also, it shouldn't really be necessary to have them.

if (authToken == null) this isn't really something this method should concern itself with. In my opinion, getUserFromDatabase should handle that case (via IllegalArgumentException, which is a RuntimeException). Although it's a case that really shouldn't happen in the first place. Why would getAuthToken return null instead of throwing a not found exception?

You should also always try to reduce the level of nesting you have, to increase readability. If you have

if (something) {
    doSomething();
    return;
} else {
    doALotMore();
}

write it as:

// we handle all the exceptional stuff here, and return
if (somethingWentWrong) {
    handleIt();
    return;
}

// the expected code goes here
toDefaultStuff();

With those two points, your code would look something like this:

@ApiMethod(name = "getUserData", path = "get_user_data")
public Bean getUserData(@Named("token") String token) {

    [...]

    try {
        authToken = ServerFacebookHelper.getAuthToken(token);
    } catch (NoConnectionException e) {
        response.setData("Exception occurred");
        return response;
    } catch (InvalidTokenException ite) {
        response.setData("Token invalid, please log in");
        return response;
    } catch (...) {
        ...
    }

    user = getUserFromDatabase(authToken);
    if (user == null) {
        user = registerUserInDatabase(authToken);
        // removed setData duplication
    }
    response.setData(user.getPersonalisedWelcome);
    return response;
}

if (user == null) also doesn't seem like a good way of handling it. Why is user null? Currently, we don't know, it could be a number of reasons (user not found, problems with the database conncetion, incorrect query, etc). This is why returning null for functionality is generally not a great idea, because the caller will have to guess quite a bit.

One solution would be to add a isUserInDatabase(authToken), but that would add additional db queries.

You could also throw a UserNotFound exception, but it doesn't seem like an exceptional situation, so that is also not ideal (at least in this situation; generally, not finding a user would be exceptional for a getUserFromDatabase method).

Another option would be to add an exists method to your user class, but again, that doesn't seem all that clean either.

The question is what the difference between registering and getting really is (and what data is set in user; for registering, this seems to only be the token, which means that you get an incomplete user object returned, which also doesn't seem ideal), and why you don't handle it in completely different methods (ie why getUserData does something - registering a user - if the user doesn't exist already (instead of having another method for that)).

Answer 2

I am mainly a C# .NET developer, but Java code seems quite clear for me. Your code seems fine, but it might be slightly improved:

1. Here:

    catch (Exception e) {
       response.setData("Exception occurred");
       return response;
   }
   

   You might be interested in logging exception's details somewhere.

2. Reduce nesting (less "spaghetti", more readable). E.g.:

   if (authToken == null) {
       response.setData("Token invalid, please log in");
       return response;
   } else {
   

   else can be removed, as return will skip all the rest of the code

   Thus, the code might look like this:

   @ApiMethod(name = "getUserData", path = "get_user_data")
   public Bean getUserData(@Named("token") String token) {
   
      Bean response = new Bean();
      FacebookAuthToken authToken;
      User user;
      String userPersonalisedWelcome;
   
      try {
          authToken = ServerFacebookHelper.getAuthToken(token);
      } catch (Exception e) {
          response.setData("Exception occurred");
          return response;
      }
   
      if (authToken == null) {
         response.setData("Token invalid, please log in");
         return response;
      }
   
      user = getUserFromDatabase(authToken);
      if (user == null) 
         // what happens if something fails here?
         user = registerUserInDatabase(authToken);
   
      response.setData(user.getPersonalisedWelcome);
      return response;
   }
   

Answer 3

Is better to write two private (or protected if you want override in future) methods to get a valid auth token and a valid user.

@ApiMethod(name = "getUserData", path = "get_user_data")
public Bean getUserData(@Named("token") String token) {

    Bean response = new Bean();
    FacebookAuthToken auth;
    User user;

    try {
        auth = getAuth(token);
    } catch (IllegalArgumentException e){
        response.setData("Token invalid, please log in");
        return response;
    } catch (Exception e) {
        response.setData("Exception occurred");
        return response;
    }
    user = getUser(auth);
    response.setData(user.getPersonalisedWelcome);
    return response;
}

private FacebookAuthToken getAuth(String token) raise Exception{
    FacebookAuthToken auth = ServerFacebookHelper.getAuthToken(token);
    if (auth == null) {
        raise IllegalArgumentException("No valid token");
    }
    return auth;
}

private User getUser(FacebookAuthToken auth) {
    User user = getUserFromDatabase(auth);
    if (user == null) {
        user = registerUserInDatabase(auth);
    }
    return user;
}

Maybe the next step could be wrap FacebookAuthToken with a factory that give to you a method to check if is valid, an error message and facebook's token.

Anyway, consider to make a class instead a method. I didn't it because I can seems overkilled here, but if your error or user handling increase is better to switch in a class.

[Edit]

I took another review to my answer and I noted that is already simple to separate error handling and real duty:

protected String welcome(String token) {
    return getUser(getAuth(token)).getPersonalisedWelcome
}

and finally your method become a neat (as say Uncle Bob Exception handling is a huge duty yet):

@ApiMethod(name = "getUserData", path = "get_user_data")
public Bean getUserData(@Named("token") String token) {

    Bean response = new Bean();

    try {
        response.setData(welcome(token));
    } catch (IllegalArgumentException e){
        response.setData("Token invalid, please log in");
    } catch (Exception e) {
        response.setData("Exception occurred");
    }
    return response;
}

Now you can use a decorator to implement the same error handling for all your beans.

One more thing : I used to catch and raise raw Exception just because you didn't say what kind of exception getAuthToken() can raise but never do that and catch exactly you know can be raised and you know how to handle it at this level.

Answer 4

In addition to all the other answers here:

1. This code:

   } else {
       user = getUserFromDatabase(authToken);
       if (user == null) {
           user = registerUserInDatabase(authToken);
           userPersonalisedWelcome = user.getPersonalisedWelcome;
           response.setData(userPersonalisedWelcome);
       } else {
           response.setData(user.getPersonalisedWelcome);
       }
       return response;
   }
   

   has duplicate code. Though @Michele did an excellent job talking about separating into methods, keep in mind that duplicate code can always be reduced. In this case:

   } else {
       user = getUserFromDatabase(authToken);
       if (user == null) {
           user = registerUserInDatabase(authToken);
       }
       userPersonalisedWelcome = user.getPersonalisedWelcome;
       response.setData(userPersonalisedWelcome);
       return response;
   }
   

   and can be further improved by removing redundant code to:

   } else {
       user = getUserFromDatabase(authToken);
       if (user == null) {
           user = registerUserInDatabase(authToken);
       }
       response.setData(user.getPersonalisedWelcome);
       return response;
   }
   

2. I don't think the other answers covered this, but here:

   try {
       authToken = ServerFacebookHelper.getAuthToken(token);
   } catch (Exception e) {
       response.setData("Exception occurred");
       return response;
   }
   

   Never catch Exception. Always catch only the exceptions that is possible to be thrown. It should be:

   try {
       authToken = ServerFacebookHelper.getAuthToken(token);
   } catch (SomeException | SomeOtherException e) { // Whatever exception can be thrown
       response.setData("Exception occurred");
       return response;
   }