API method to validate Facebook OAuth token

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):

• 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.

• Catches any exceptions (most likely an

IOException) thrown during this validation process.

• Asks the user to

log in again if their token is invalid (if (authToken == null)).

• Else if authentication has been successful, tries to get the user

record from my database.

• If it doesn't exist, adds the user to the

database. (registerUserInDatabase).

• 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

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)).

StackExchange Code Review Q#114205, answer score: 11