Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
add comment

3 Answers 3

up vote 5 down vote accepted
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;
}
share|improve this answer
    
Thank you for answer. +1 for writing which one new variable is useful and why. I agree with you. The Map can contain just Object, no int type, that's the reason I use Integer (maybe I am wrong). If the state is not in map, I want to return SUCCESS. The comparing of int and null won't work in required way. I agree with points 4 and 5, I just use it in this way to simplify code and point only to issue with objects. The rest of code was deleted, modified. –  srnka Feb 26 '13 at 15:50
1  
@srnka Why don't you use generics? Something like: Map<Integer, Integer> statePhoneNoMap = new HashMap<Integer, Integer>(). –  Quentin Pradet Feb 26 '13 at 16:01
    
I use it: Map<String, Integer> statePhoneNoMap = new HashMap<String, Integer>(); So there is Integer, no int. –  srnka Feb 26 '13 at 16:11
2  
Then, look at autoboxing. –  Quentin Pradet Feb 26 '13 at 17:37
1  
"I think (but could be wrong) that the comparison can simply use == since you are comparing ints". No, both are Integers. If we change the method variable to int, we could get a NullPointerException because the vm tries to promote a null to int. –  tb- Feb 27 '13 at 19:50
show 2 more comments

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.

share|improve this answer
    
Thank you, in first moment I thought that I design it in this way: new object and check of correctness of values in constructor. Since my code is just about parsing data from input, evaluating their correctness and put to output, I consider this as not useful - for this particular case. In global, I think it is good idea to raise the level of abstraction. –  srnka Feb 27 '13 at 9:28
    
@srnka mariosangiorgio has a very good point here. Even if you don't abstract the state into an object as he suggests, you should at least move the trimming of the state (and the phone number) outside the checkStatePhoneLen function, since you'll most likely be needing those trimmed values in other contexts anyway. –  RoToRa Feb 28 '13 at 13:54
add comment

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;
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.