Defining of new, temporary, variables or usage of already known ones?

I want to check if the length of phone number is appropriate for specified country (let's consider that only some countries have restriction, another countries accept phone number with various length). I have a Map, where the correct pairs are defined so this map can be used as a reference in the condition:

public static ErrCode checkStatePhoneLen(final String state, final String phoneNo) 
{
    String  stateTmp = state.trim();
    String  phoneTmp = phoneNo.trim();
    Integer phoneLen = new Integer(phoneTmp.length());

    if ( statePhoneNoMap.containsKey(stateTmp) && !phoneLen.equals(statePhoneNoMap.get(stateTmp)))
    {
        return ERROR;
    }
    return SUCCESS;
}

My questions are:

• Is it better to use temporary variables or directly usage of already existed object? I can just use "state.trim()" instead of creating the variable state_tmp and so on. I think that advantages of the solution with temporary variables are better readability and debugging but disadvantages are the effort to create new variable by runtime (or is it optimized someway by compiler?) and more rows of code (but I prefer readability factor more than number of rows factor).

-
is it better to check if map contains the key and then compare, or to get value for given key and then check if it is not null and compare them? As following example:

Integer definedLen = (Integer) statePhoneNoMap.get(stateTmp);

if (definedLen != null && !definedLen.equals(phoneLen)){

In this code sample, there is needed one more variable, but the condition is clearer. And, there is just one operation upon map (get()) instead of two in previous code (containsKey(), get())

What is better solution? How would you modify this function?

public static ErrCode checkStatePhoneLen(final String state, final String phoneNo) 
{
    String  stateTmp = state.trim();
    String  phoneTmp = phoneNo.trim();

The issue is that stateTmp is less readable than state.trim(), so if you want to create a new variable, make sure that the name carries your intent. You can go for "normalizedState", but since you're only trimming (and are not normalizing capitalization for example) then a variable is useless.

Integer phoneLen = new Integer(phoneTmp.length());

This one is useful! phoneLen is clearer than phoneNo.trim().length(). This answers your first question: it depends! Use new variable names when they do make thing clearer, but never use dummy names like phoneTmp, phone1, and so on. By the way, I may be mistaken, but are you certain Integer is necessary? Java should do autoboxing. I would simply write int phoneLen = phoneNo.trim().length().

if (statePhoneNoMap.containsKey(stateTmp) && !phoneLen.equals(statePhoneNoMap.get(stateTmp)))
    {
        return ERROR;
    }
    return SUCCESS;

• To answer your second question, you don't need to explicitely check for null: it's simpler to compare phoneLen and statePhoneNoMap.get(stateTmp) directly. If the latter is null, then the comparison will return false. If phoneLen is null, you don't want to return a value but throw an exception anyway, and this is what happens with your current code because null.equals(...) throws.

• It makes more sense to check for success and return ERROR if something went wrong.

• If you have an ErrCode type instead of a boolean, you have to return more explicit codes! Otherwise just return the condition directly.

The code becomes:

public static ErrCode checkStatePhoneLen(final String state, final String phoneNo) 
{
    int phoneLen = phoneNo.trim().length();
    int stateTrimmed = state.trim();

    if (statePhoneNoMap.containsKey(stateTrimmed)
        && phoneLen == statePhoneNoMap.get(stateTrimmed))
    {
        return SUCCESS;
    }
    return ERROR;
}

StackExchange Code Review Q#23163, answer score: 7