Formatting phone number without knowing locale

Question

I am trying to format supplied phone numbers for a web application. Users in general will be supplying US-based phone numbers, but it is possible for some users to input a phone number from another country.

Here are my assumptions:

• Default to US if the number of digits is 7 or 10 after stripping non-digits. If not, just return the String
• Use tested external libraries where possible since speed isn't an utmost concern (the service can be run in the background if it is really that slow).
• Prefer an external library over a custom regex.

For the first assumption, I am thinking that if there is another country that has phone numbers with 7 or 10 digits, it's not the end of the world to have it formatted in US format.

Here's my code anyway, using Google's libphonenumber:

public static String formatPhoneNumber(String phoneNumber, Locale locale) throws NumberParseException {
    PhoneNumberUtil phoneUtil = PhoneNumberUtil.getInstance();
    Phonenumber.PhoneNumber usNumber = phoneUtil.parse(phoneNumber, locale.getCountry());
    return phoneUtil.format(usNumber, PhoneNumberUtil.PhoneNumberFormat.NATIONAL);
}

public static String formatUSPhoneNumber(String phoneNumber) throws NumberParseException {

    //First strip out any non-digits
    phoneNumber = stripNonDigits(phoneNumber);

    phoneNumber = (phoneNumber == null) ? BaseConstants.EMPTY_STRING : phoneNumber;
    if ((phoneNumber.length() != 7) && (phoneNumber.length() != 10)) {
        return phoneNumber;
    }
    return formatPhoneNumber(phoneNumber, Locale.US);
}

I am adding this method to be comprehensive, but it's not directly needed for review (credit to SO):

public static String stripNonDigits(final CharSequence input){
    final StringBuilder sb = new StringBuilder(input.length());

    for(int i = 0; i < input.length(); i++){
        final char c = input.charAt(i);
        if(c > 47 && c < 58){
            sb.append(c);
        }
    }
    return sb.toString();
}

The associated JUnit tests:

@Test
public void testFormatPhoneNumber() throws NumberParseException {
  assertEquals(FormUtils.formatPhoneNumber("2122222222", Locale.US), "(212) 222-2222");
  assertEquals(FormUtils.formatPhoneNumber("2122222222", Locale.CANADA), "(212) 222-2222");
  assertEquals(FormUtils.formatPhoneNumber("(021)11111111", Locale.GERMANY), "0211 1111111");
}

@Test
public void testFormatUSPhoneNumber() throws NumberParseException {
  assertEquals(FormUtils.formatUSPhoneNumber("2122222222"), "(212) 222-2222");
  assertEquals(FormUtils.formatUSPhoneNumber("92122222222"), "92122222222");
  assertEquals(FormUtils.formatUSPhoneNumber("(212)2222222"), "(212) 222-2222");
  assertEquals(FormUtils.formatUSPhoneNumber("(212)222-2222"), "(212) 222-2222");
  assertEquals(FormUtils.formatUSPhoneNumber("(212) 222-2222"), "(212) 222-2222");
}

Is there a better way to do this? Are there tests I should add?

Answer 1

Some improvements...

Variable re-assignment and null-check

You are re-assigning phoneNumber twice, and unfortunately I think you don't even need it at all.

First, it's generally not a nice idea to re-assign the method argument because careless mistakes may result in you inadvertently changing the instance referenced by it somewhere inside the method, when you're expecting the original input. Even if you don't explicitly final method parameters, it's simpler to always treat method arguments as an invariant so that you know what the method is processing on/with. Therefore, you should simply assign the output of stripNonDigits() to a new variable, and you can inline the new variable in the subsequent calls:

public static String formatPhoneNumber(String phoneNumber) throws NumberParseException {
    String strippedNumber = stripNonDigits(phoneNumber);
    return strippedNumber.length() != 7 && strippedNumber.length() != 10 ?
                strippedNumber : formatAsUSPhoneNumber(strippedNumber);
}

Now, you don't need a null-check (it's not in the right place anyways - it should be done at the start), and I renamed the main method as formatAsUSPhoneNumber(). I feel that formatUSPhoneNumber() is suggesting that it is formatting a US phone number into another representation.

stripNonDigits()

If stripNonDigits() should be used only by formatAsPhoneNumber(), consider making them private static. Ok, so even if you prefer not to use regex (fair point), how about Character.isDigit()? I feel that it's easier to read than the condition you have, short of the one-liner regex:

// replace all non-digits with an empty String
return input.toString().replaceAll("\\D+", "");