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'm using this code to bring my phone numbers in a consistent format.

Desired: +(country code)phone number

Possible patterns:

01721234567 -> change to desired pattern

00491234567 -> change to desired pattern

+4912345678 -> do nothing, already desired pattern

This is what I use:

String number = allContactNumbers.get(i).get(j);
number = number.replaceAll("[^+0-9]", ""); // All weird characters such as /, -, ...

String country_code = getResources().getString(R.string.countrycode_de);

if (number.substring(0, 1).compareTo("0") == 0 && number.substring(1, 2).compareTo("0") != 0) {
    number = "+" + country_code + number.substring(1); // e.g. 0172 12 34 567 -> + (country_code) 172 12 34 567
}

number = number.replaceAll("^[0]{1,4}", "+"); // e.g. 004912345678 -> +4912345678

For some reason I'm not happy with it though. I hope there is somebody to tell me, whether my code is written properly!

share|improve this question
    
Is there a reason not to remove all + and all leading zeroes as step 1, and then just add the leading + at the end? No country code begins with 0. –  Edward Apr 3 at 1:40
    
like make every number begin with [1-9] and then simply add +? what would be the easiest way to do this? –  Philip Apr 3 at 1:42
    
@Edward - the existing code appears to assume a number beginning with "0" is a national number and prefixes it with a defined national prefix code. While no country code begins with 0, several countries use 0 as the prefix for national dialling (i.e. the same function a prefix of "1" serves for North American numbers). –  Jules Apr 3 at 4:31
    
Given how localized telephone number formats can be, I think you'll get better answers if you can at least indicate which country/region you are focusing on. There are countries where phone numbers certainly don't start with 1, 0, 00 or nation-wide prefixes for that matter. –  h.j.k. Apr 3 at 6:18
1  
There is a very good library written by Google which deals with a lot of the hard problems around number formatting: code.google.com/p/libphonenumber –  Colin Pickard Apr 3 at 12:44
show 4 more comments

4 Answers 4

up vote 3 down vote accepted

There are two aspects to this, the general design, and the implementation.

Implementation

The entire things should be extracted as a function. The first line of code:

String number = allContactNumbers.get(i).get(j);

indicates that this code is being run inside a loop (i,j). You need to extract it to be:

String number = normalizePhoneNumber(allContactNumbers.get(i).get(j));

and the function would look something like:

public String normalizePhoneNumber(String number) {
    ......
    return normalized;
}

Right, about that function, putting your code in it ends up with:

public String normalizePhoneNumber(String number) {

    number = number.replaceAll("[^+0-9]", ""); // All weird characters such as /, -, ...

    String country_code = getResources().getString(R.string.countrycode_de);

    if (number.substring(0, 1).compareTo("0") == 0 && number.substring(1, 2).compareTo("0") != 0) {
        number = "+" + country_code + number.substring(1); // e.g. 0172 12 34 567 -> + (country_code) 172 12 34 567
    }

    number = number.replaceAll("^[0]{1,4}", "+"); // e.g. 004912345678 -> +4912345678

    return number;
}

Design

OK, about the design.... I believe this is a problem. Handling phone numbers is much more complicated than what you have.... it is really a challenging problem.

It is easy enough to strip off the junk, but it gets hard really fast. For example, this is the the Queen of England's land-line number (not kidding):

Public Information Officer
Buckingham Palace
London SW1A 1AA
Tel (during 9am - 5pm (GMT) Monday to Friday): (+44) (0)20 7930 4832. Please note, calls to this number may be recorded.

As a Canadian, the international dialing code for this would be:

01144207934832

How will your script translate that in to:

+442079305832

I think you need to revise your plan for this problem, it is more complicated than you realize.

share|improve this answer
    
I dont see why you have to dial 01144207934832 from canada instead of +442079305832. If this number: ` (+44) (0)20 7930 4832` is provided, my script would first make it: +4402079304832 then in the second step it would erase the plus, delete all zeroes in the beginning (there are none in this example) then add the + again, this would lead to +4402079304832, which is wrong, since there is a 0 after the country code, BUT nobody would have it in his adressbook like this. and if, its their fault. –  Philip Apr 3 at 2:17
1  
The 011 is the international direct dial (IDD) number that must be dialed before the country code in Canada and the US. In Germany the IDD is 0 and the UK the IDD is 00. –  Edward Apr 3 at 2:37
add comment

I don't have a handy way to check this code right now, but I believe it could be done like this:

// only keep digits
number = number.replaceAll("[^0-9]", ""); 
// trim leading zeroes
number = "+" + number.replaceFirst("^0*(.*)","$1");
share|improve this answer
    
but in this example you dont check whether country code is given or not –  Philip Apr 3 at 2:18
    
That's correct. Reducing a phone number with embedded spaces and parentheses and possibly leading zeroes is relatively simple. Picking out the country code is, as the other answer notes, much more complex. For example +1 is actually not a country code but a signifier of the North American numbering plan. Further, the US and Canada both use +1 with no other signifier. And it's not even tidy geographically. A number beginning with +1 809 is the Dominican Republic but Haiti (on the other end of the same island!) is +509. There's no simple way to untangle it. –  Edward Apr 3 at 2:29
1  
@Edward: "For example +1 is actually not a country code but a signifier of the North American numbering plan." -- That's a distinction without a difference. From an outsider's perspective it's simply a country code that corresponds to more than one country. It is isomorphic to a country code of "1" with a national dialing prefix of "1" being ignored. The fact that multiple countries fall within the code is irrelevant to almost all purposes you might have for this kind of algorithm. –  Jules Apr 3 at 4:40
    
@Jules: Many people, even in Europe, consider Canada and the US to be different countries. :) –  Edward Apr 3 at 11:22
add comment

The biggest problem I have is that your code assumes a particular pattern for national numbers and international numbers that really ought to be end-user configurable. You should have preferences for international prefix (probably defaulting to "00"), for local national code (which you are currently retrieving from a resource, which should instead be used to set the default value for the preference) and for local number national prefix (which you currently have hardwired to "0"). Then, assuming you have a method to retrieve a preference with the signature getPreferenceString(String preferenceName, String defaultValue), the algorithm would look something like:

String originalNumber = (whatever code you need to get the original number)
String internationalPrefix = getPreferenceString(PREF_INTERNATIONAL_PREFIX, "00");
String defaultCountryCode = getPreferenceString(PREF_DEFAULT_COUNTRY_CODE, 
    getResources().getString(R.string.countrycode_de));
String defaultCountryNationalPrefix = getPreferenceString(PREF_NATIONAL_PREFIX, "0");

// strip any non-significant characters
String number = originalNumber.replaceAll("[^0-9+]", ""); 
// check for prefixes
if (number.startsWith ("+")) // already in desired format
    return number;
if (number.startsWith(internationalPrefix)) 
    return number.replace(internationalPrefix, "+");
else if (number.startsWith(defaultCountryNationalPrefix))
    return number.replace(defaultCountryNationalPrefix, "+" + defaultCountryCode);
else 
    throw new IllegalArgumentException ("Number " + originalNumber + " does not have either an international or a national prefix");

Note that this code can throw an exception if the phone number does not conform to expectations, e.g. if it lacks either an international or national prefix. In such a case, either the preferences are set incorrectly, or the phone has local numbers in its database, which the system I've described doesn't have enough information to translate. Such numbers wouldn't be usable on a mobile phone, as mobile networks do not typically support dialing local numbers, but I don't know that there aren't desk phones with Android installed on them somewhere...

As a side comment, you're doing your string comparisons in an odd way. You should use string.equals("expected value") not string.compareTo("expected value") == 0 to check for equality (and !string.equals(...) rather than ... != 0 for inequality) because (1) it's easier to read and (2) it's slightly more efficient at run time.

Also, there's a method startsWith(String). Use it, rather than extracting a substring and comparing the result, which is error-prone (if you get the length of the substring wrong the comparison will always fail, or if the string you're comparing is shorter than the prefix you're checking for an IndexOutOfBoundsException will be thrown).

share|improve this answer
add comment

If you are going to use regular expressions, use them effectively so that you don't have to resort to low-level string manipulation. In particular, make use of capturing parentheses to figure out what was found during matching.

public class TelephoneNumberCanonicalizer {
    private static final Pattern EUROPEAN_DIALING_PLAN = Pattern.compile("^\\+|(00)|(0)");
    private final String countryCode;

    public TelephoneNumberCanonicalizer(String countryCode) {
        this.countryCode = countryCode;
    }

    public String canonicalize(String number) {
        // Remove all weird characters such as /, -, ...
        number = number.replaceAll("[^+0-9]", "");

        Matcher match = EUROPEAN_DIALING_PLAN.matcher(number);
        if (!match.find()) {
            throw new IllegalArgumentException(number);
        } else if (match.group(1) != null) {     // Starts with "00"
            return match.replaceFirst("+");
        } else if (match.group(2) != null) {     // Starts with "0"
            return match.replaceFirst("+" + this.countryCode);
        } else {                                 // Starts with "+"
            return number;
        }
    }
}

Test case

public static void main(String[] args) {
    String[] testCases = new String[] {
        "01721234567",
        "00491234567",
        "+4912345678"
    };
    TelephoneNumberCanonicalizer german = new TelephoneNumberCanonicalizer("49");
    for (String testCase : testCases) {
        System.out.printf("%s -> %s\n", testCase, german.canonicalize(testCase));
    }
}

Output

01721234567 -> +491721234567
00491234567 -> +491234567
+4912345678 -> +4912345678
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.