TimeZone computing method I feel like there is a code smell in this. Can someone help me to refactor/optimize it?

protected TimeZone resolveTimeZone() {
    IUserPreferences preferences = userProfileService.getCurrentUserPreferences();
    if (preferences != null) {
        String timeZone = preferences.getPreferredTimeZone();
        if (timeZone != null &amp;&amp; !"".equals(timeZone))
            return TimeZone.getTimeZone(timeZone);
        else return defaultTimeZone();
    }
    else return defaultTimeZone();

}

public TimeZone defaultTimeZone() {
    Locale defaultLocale = new Locale(appConfiguration.getDefaultLocale());
    Calendar calendar = Calendar.getInstance(defaultLocale);
    return calendar.getTimeZone();
}
Some immediate improvement ideas are possible at first glance:


Avoid duplicated code: defaultTimeZone() appears twice. It's easy to factor that out by changing the branching logic: keep checking the requirements (preferences != null, the preference value not null, and so on) until you find a usable value and return. To handle all other (failure) execution paths, use a single return at the end of the method
Use the .isEmpty() method to check if a string is empty. This is better than !"".equals(value), which means literally "is this value the same as """. Using isEmpty is natural.
The variable named timeZone suggests it's an instance of TimeZone. That's misleading, I would rename it to something else to clarify.


Applying the above:

protected TimeZone resolveTimeZone() {
    IUserPreferences preferences = userProfileService.getCurrentUserPreferences();
    if (preferences != null) {
        String timeZoneText = preferences.getPreferredTimeZone();
        if (timeZoneText != null &amp;&amp; !timeZoneText.isEmpty()) {
            return TimeZone.getTimeZone(timeZoneText);
        }
    }
    return defaultTimeZone();
}


On closer look, there are a few more things I don't really like:


The defaultTimeZone method sticks out a little from other getter calls in the posted code: getCurrentUserPreferences, getPreferredTimeZone, getDefaultLocale, ... Perhaps it would be better to be consistent with those and rename the method to getDefaultTimeZone
Calling Calendar.getInstance is costly. If the default timezone will not change during the lifetime of the application, then it would be better to initialize it once during startup, and reuse it afterwards.
The name resolveTimeZone is a bit vague. What does it mean, "resolve", really? Looking at the implementation, it's not as clear as it can be.


I would recommend to split the method like this:

protected TimeZone getTimeZone() {
    TimeZone preferredTimeZone = getPreferredTimeZone();
    if (preferredTimeZone != null) {
        return preferredTimeZone;
    }
    return defaultTimeZone();
}

private TimeZone getPreferredTimeZone() {
    IUserPreferences preferences = userProfileService.getCurrentUserPreferences();
    if (preferences != null) {
        String timeZoneText = preferences.getPreferredTimeZone();
        if (timeZoneText != null &amp;&amp; !timeZoneText.isEmpty()) {
            return TimeZone.getTimeZone(timeZoneText);
        }
    }
    return null;
}


Here, the names and implementations of the methods are more natural:


getTimeZone: looking at the implementation it's perfectly clear what's going on: use the preferred timezone if possible, otherwise the default. Nice and simple.
getPreferredTimeZone: from the method name, it comes natural that the method tries to figure out a timezone from the user configuration and return null otherwise.


Alas, this does have the drawback of an extra if condition compared to the original. However, the cost of the extra call should be negligible in practice. This may be a matter of taste. I think the value of clarity in the design outweighs the cost of that extra if (unless performance is absolutely critical, of course).