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

Question

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?

Answer 1

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;

1. 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.
2. It makes more sense to check for success and return ERROR if something went wrong.
3. 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;
}

Answer 2

You should raise the level of abstraction and define the specific types State and PhoneNumber.

In this way you can move the validation of the data where you build the objects removing all the cruft related to the fact that you use strings to represent higher level concepts.

You then need to put in PhoneNumber a method that returns the corresponding state (maybe using a static Map<Prefix, State> it is enough, even it is a little dirty).

The method you need then turns into checking that the returned State is equal to the one passed to the function.

Answer 3

I agree in a lot of thoughts from Quentin Pradet and mariosangiorgio.

I will add some other viewpoint:

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

My personal rule is: If in doubt, create a variable with a good name. This leads to 2 results in this case:
1) I would avoid using tmp inside the variable. Instead of stateTmp use trimmedState or stateTrimmed. A name should always have a meaning. Temp or tmp is one of the worst choices (behind data). Same goes for phoneTmp.

2a) I would not use a new variable for trimmed state. It is very clear from state.trim() what we have.
2b) I would use the new variable final int phoneNumberLength = phoneNumber.trim().length(); This describes what we have. It is not directly clear from phoneNumber.trim().length(), one have to think what is happening here.

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?

I would get the value and check for null. My usual pattern is:

variable = ...
if (something is wrong with variable){
    exit / failure handling
}
continue with normal code

This reduces branches, which reduces time finding out in which branch we are and what is the current state. It is always a bad sign if your indentation is somewhere in the 5. or 6. level. It is not so obvious from this small example, but will easily count on more complex code.

---

ErrCode

If it is an error code, name it ErrorCode. There is most probably not any reason to abbreviate it to ErrCode. Even more, if you have only 2 error codes, you should continue using boolean and if you have more complex things, it could be interesting to use exceptions. I do not like to return error codes, but this are probably just the results of bad experiences from C.

---

public static ErrCode checkStatePhoneLen(final String state, final String phoneNo)

Again, avoid abbreviations. And try to find method names, where you could "read" the source code.
if (checkStatePhoneLen(...)) could not be read in a normal way, because you have to look up what is the expected result. Is it true or is it false? If you use a name like isPhoneNumberCorrectForState, you will have if(isPhoneNumberCorrectForState(...)) which is completely clear for the reader.
If ErrCode is an enum and you do a comparison like if (checkStatePhoneLen(...) == ERROR) then you should name it getErrorCodeForPhoneNumberCorrectnessForState. Which starts to be clumpy, which is a good sign to refactoring as mariosangiorgio suggested.

---

If you put everything together, it could be something like this:

public static boolean isPhoneNumberCorrectForState(final String phoneNumber, final String state) {
    final int phoneNumberLength = phoneNumber.trim().length();

    final Integer maximumPhoneNumberLength = mapStateToMaximumPhoneNumberLength.get(state.trim());

    if (maximumPhoneNumberLength != null && phoneNumberLength > maximumPhoneNumberLength) {
        return false;
    }
    return true;
}