TimeZone computing method

Question

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 && !"".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();
}

Answer 1

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

Answer 2

I would rewrite resolveTimeZone like this:

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

Notice the fact that I removed the else branch from the innermost if. It was not needed. In case the timezone check fails, the function will return at the last return statement anyway.

I also replaced the yoda conditional there. I personally dislike yoda conditionals, and I believe they do not add any sort of value, especially in your case. timeZone.equals("") is read as "timezone equals empty string" whereas "".equals(timeZone) is read as "empty string equals timeZone". Logically equivalent since equality is commutative, but the second version is hardly natural for most people.

I also removed the empty line before the last brace. It kinda showed a lack of attention to detail, and it broke the rather consistent code surrounding it. I guess it's safe to say it was the first code smell I spotted.

I have no big issues with defaultTimeZone(). Perhaps the fact that the name would be more appropriate for a field, rather than a function performing an action?

All of the above aside, your code is quite clean and easy to parse.

Answer 3

It is a matter of taste, but at lesst less errorprone, but you should consider using braces {} for single if statements too.

That beeing said, let us face the resolveTimeZone() method.

The else part can be removed completly, because if the condition evaluates to true it won' t be reached.

The method can be refactored to

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