patternjavaMinor
Formatting phone number without knowing locale
Viewed 0 times
formattingwithoutnumberknowinglocalephone
Problem
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:
libraries where possible since speed isn't an utmost concern (the
service can be run in the background if it is really that slow).
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:
I am adding this method to be comprehensive, but it's not directly needed for review (credit to SO):
The associated
```
@Test
public void testFormatPhoneNumber() throws Num
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 47 && c < 58){
sb.append(c);
}
}
return sb.toString();
}The associated
JUnit tests:```
@Test
public void testFormatPhoneNumber() throws Num
Solution
Some improvements...
Variable re-assignment and
You are re-assigning
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
Now, you don't need a
If
Variable re-assignment and
null-checkYou 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+", "");Code Snippets
public static String formatPhoneNumber(String phoneNumber) throws NumberParseException {
String strippedNumber = stripNonDigits(phoneNumber);
return strippedNumber.length() != 7 && strippedNumber.length() != 10 ?
strippedNumber : formatAsUSPhoneNumber(strippedNumber);
}// replace all non-digits with an empty String
return input.toString().replaceAll("\\D+", "");Context
StackExchange Code Review Q#88481, answer score: 3
Revisions (0)
No revisions yet.