JAVA: libphonenumber 4.5

JAVA: libphonenumber 4.5
Committed: http://code.google.com/p/libphonenumber/source/detail?r=415

[lararennie, 2012-01-19 11:17:39]

http://codereview.appspot.com/5532089/diff/1/java/demo/src/com/google/phonenu...
File java/demo/src/com/google/phonenumbers/PhoneNumberParserServlet.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/demo/src/com/google/phonenu...
java/demo/src/com/google/phonenumbers/PhoneNumberParserServlet.java:55: String
defaultCountry = "ZZ"; // Default defaultCountry to ZZ if nothing is entered.
As discussed ZZ and null behave the same, so you don't have to worry about
changing this - it's not the same as the internal demo anyway code-wise but it
already behaved appropriately if people didn't enter anything.

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
File java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:78: //
There are roughly 220 of them and we set the initial capacity of the HashSet to
300 to offer a
There are 240 of them now. Do we want to change this?

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:700:
private void loadMetadataForCountryCodeFromFile(String filePrefix, int
countryCallingCode) {
These functions seem to look quite similar: I wonder if you could have a
loadMetadata function that takes in a file-name, map to put it in, and a key?
The unfortunate bit is that one map is Integer to Metadata and the other is
String to Metadata so it would need to be Object to Metadata in the method
prefix.... Thoughts?

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:1944:
if (!isValidRegionCode(regionCode)) {
I notice internally we just check for null, since looking it up in the map will
return null anyway if it's not present, so it is faster that way. But it is
inconsistent :/ but I guess the whole method is so it's not so much a problem...

Could we make it here just check null, and then loadMetadataForRegionFromFile
can cope gracefully if it doesn't find an appropriate file? I guess that would
be slower though cos it'd be looking for files that don't exist. Meeeeeeh. Ok,
let's do nothing.

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:1955:
PhoneMetadata getMetadataForNonGeographicalRegion(int countryCallingCode) {
Should you not also check here that the countryCallingCode is valid before doing
this?

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:2031:
return null;
Internally we log a warning here if no regions are found, since that means the
PhoneNumber has an invalid country calling code. Can we add that here too? Does
it happen too often on Android? It used to because we used to do it for invalid
numbers that had a correct country calling code, but then we fixed the logging
to not do that...

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
File
java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java:171:
assertEquals(RegionCode.AR, metadata.getId());
Delete line 171 while you're here

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
File java/libphonenumber/test/com/google/i18n/phonenumbers/RegionCode.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
java/libphonenumber/test/com/google/i18n/phonenumbers/RegionCode.java:23: static
final String UN001 = "001";
Can you add a comment about this being the official code for non-geographical
phone number authorities?

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt
File java/release_notes.txt (right):

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt#newcode1
java/release_notes.txt:1: Janurary 18th, 2012: libphonenumber-4.5
You have spelt January wrong. Plus it won't be the 18th any more when you submit
:)

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt#newcode7
java/release_notes.txt:7: - Update demo to set defaultCountry to "ZZ" when it is
left empty.
And you can remove this line.

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt#newcode13
java/release_notes.txt:13: - Geocoding data updates: en/1, en/54, es/54
We did countries before, rather than file names or country calling codes. If you
want to keep these, maybe put both?

[Shaopeng, 2012-01-19 14:47:17]

http://codereview.appspot.com/5532089/diff/1/java/demo/src/com/google/phonenu...
File java/demo/src/com/google/phonenumbers/PhoneNumberParserServlet.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/demo/src/com/google/phonenu...
java/demo/src/com/google/phonenumbers/PhoneNumberParserServlet.java:55: String
defaultCountry = "ZZ"; // Default defaultCountry to ZZ if nothing is entered.
Ok, I don't mind reverting this from the change.

On 2012/01/19 11:17:39, lararennie wrote:
> As discussed ZZ and null behave the same, so you don't have to worry about
> changing this - it's not the same as the internal demo anyway code-wise but it
> already behaved appropriately if people didn't enter anything.

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
File java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:78: //
There are roughly 220 of them and we set the initial capacity of the HashSet to
300 to offer a
On 2012/01/19 11:17:39, lararennie wrote:
> There are 240 of them now. Do we want to change this?

Done.

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:700:
private void loadMetadataForCountryCodeFromFile(String filePrefix, int
countryCallingCode) {
I have definitely thought about it, but put that off due to the reason you
mentioned. Another thing I can do is to merge them into one method of
loadMetadataFromFile(String filePrefix, String regionCode, int
countryCallingCode), but I would need to add a few if-statement inside to do
different things for the two cases. Let me know if you think that is better.

On 2012/01/19 11:17:39, lararennie wrote:
> These functions seem to look quite similar: I wonder if you could have a
> loadMetadata function that takes in a file-name, map to put it in, and a key?
> The unfortunate bit is that one map is Integer to Metadata and the other is
> String to Metadata so it would need to be Object to Metadata in the method
> prefix.... Thoughts?

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:1944:
if (!isValidRegionCode(regionCode)) {
Ok.

On 2012/01/19 11:17:39, lararennie wrote:
> I notice internally we just check for null, since looking it up in the map
will
> return null anyway if it's not present, so it is faster that way. But it is
> inconsistent :/ but I guess the whole method is so it's not so much a
problem...
> 
> Could we make it here just check null, and then loadMetadataForRegionFromFile
> can cope gracefully if it doesn't find an appropriate file? I guess that would
> be slower though cos it'd be looking for files that don't exist. Meeeeeeh. Ok,
> let's do nothing.

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:1955:
PhoneMetadata getMetadataForNonGeographicalRegion(int countryCallingCode) {
Yes I should. Thanks for noticing!

On 2012/01/19 11:17:39, lararennie wrote:
> Should you not also check here that the countryCallingCode is valid before
doing
> this?

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:2031:
return null;
Ok, I have added that. They are generally against too much logging. Since we
have changed the logic here this shouldn't log that much.

On 2012/01/19 11:17:39, lararennie wrote:
> Internally we log a warning here if no regions are found, since that means the
> PhoneNumber has an invalid country calling code. Can we add that here too?
Does
> it happen too often on Android? It used to because we used to do it for
invalid
> numbers that had a correct country calling code, but then we fixed the logging
> to not do that...

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
File
java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java:171:
assertEquals(RegionCode.AR, metadata.getId());
On 2012/01/19 11:17:39, lararennie wrote:
> Delete line 171 while you're here

Done.

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
File java/libphonenumber/test/com/google/i18n/phonenumbers/RegionCode.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/test/com/goo...
java/libphonenumber/test/com/google/i18n/phonenumbers/RegionCode.java:23: static
final String UN001 = "001";
On 2012/01/19 11:17:39, lararennie wrote:
> Can you add a comment about this being the official code for non-geographical
> phone number authorities?

Done.

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt
File java/release_notes.txt (right):

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt#newcode1
java/release_notes.txt:1: Janurary 18th, 2012: libphonenumber-4.5
On 2012/01/19 11:17:39, lararennie wrote:
> You have spelt January wrong. Plus it won't be the 18th any more when you
submit
> :)

Done.

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt#newcode7
java/release_notes.txt:7: - Update demo to set defaultCountry to "ZZ" when it is
left empty.
On 2012/01/19 11:17:39, lararennie wrote:
> And you can remove this line.

Done.

http://codereview.appspot.com/5532089/diff/1/java/release_notes.txt#newcode13
java/release_notes.txt:13: - Geocoding data updates: en/1, en/54, es/54
On 2012/01/19 11:17:39, lararennie wrote:
> We did countries before, rather than file names or country calling codes. If
you
> want to keep these, maybe put both?

Changed to be just countries.

[lararennie, 2012-01-19 15:00:52]

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
File java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java
(right):

http://codereview.appspot.com/5532089/diff/1/java/libphonenumber/src/com/goog...
java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java:700:
private void loadMetadataForCountryCodeFromFile(String filePrefix, int
countryCallingCode) {
On 2012/01/19 14:47:17, Shaopeng wrote:
> I have definitely thought about it, but put that off due to the reason you
> mentioned. Another thing I can do is to merge them into one method of
> loadMetadataFromFile(String filePrefix, String regionCode, int
> countryCallingCode), but I would need to add a few if-statement inside to do
> different things for the two cases. Let me know if you think that is better.

I think it would be worth a try :)

> 
> On 2012/01/19 11:17:39, lararennie wrote:
> > These functions seem to look quite similar: I wonder if you could have a
> > loadMetadata function that takes in a file-name, map to put it in, and a
key?
> > The unfortunate bit is that one map is Integer to Metadata and the other is
> > String to Metadata so it would need to be Object to Metadata in the method
> > prefix.... Thoughts?
>