Adding handling for AutocompletePrediction's structured formatting. by domesticmouse · Pull Request #330 · googlemaps/google-maps-services-java

domesticmouse · 2017-08-14T03:21:59Z

This is a fix for #244

domesticmouse · 2017-08-14T03:23:34Z

/cc @apjanke do you want to review my javadoc for sanity? Thanks!

apjanke

I've added some minor style suggestions, mostly about redundant words and shortening things to noun phrases.

apjanke · 2017-08-15T01:53:38Z

src/main/java/com/google/maps/model/AutcompleteStructuredFormatting.java

+package com.google.maps.model;
+
+/** Represents the Structured Formatting field of an {@see AutocompletePrediction}. */
+public class AutcompleteStructuredFormatting {


Typo: Should this be "AutocompleteStructuredFormatting" instead of "AutcompleteStructuredFormatting"?

I would make this just The structured formatting info for a {@link com.google.maps.model.AutocompletePrediction}. IMHO, "represents" is redundant, since all data classes represent things, and we shouldn't call it a "field", because it's rather the kind of data that will be stored in the field.

Done, and yes, that's quite a typo right there...

apjanke · 2017-08-15T01:57:52Z

src/main/java/com/google/maps/model/AutcompleteStructuredFormatting.java

+/** Represents the Structured Formatting field of an {@see AutocompletePrediction}. */
+public class AutcompleteStructuredFormatting {
+
+  /** Contains the main text of a prediction, usually the name of the place. */


IMHO, all these "contains" are also redundant, because all variables contain data. How about simplify this to just a noun phrase, like The main text of a prediction, usually the name of the place.?

apjanke · 2017-08-15T01:58:46Z

src/main/java/com/google/maps/model/AutcompleteStructuredFormatting.java

+  /** Contains the main text of a prediction, usually the name of the place. */
+  public String mainText;
+
+  /** Contains an array of where the query matched the returned main text. */


I'd just do Where the query matched the returned main text.. "Contains" is redundant, and the fact that it's an array is apparent from the field's type.

apjanke · 2017-08-15T01:59:42Z

src/main/java/com/google/maps/model/AutocompletePrediction.java

  }

+  /**
+   * Describe the location of the entered term in the prediction result text, so that the term can


I'd omit "describe", and pluralize "location". Maybe, The locations of the entered term in....

apjanke · 2017-08-15T02:00:32Z

src/main/java/com/google/maps/model/AutocompletePrediction.java

  public MatchedSubstring matchedSubstrings[];

+  /**
+   * Structured formatting contains a description of how the autocomplete query matched the returned


I'd simplify to A description of how the autocomplete query matched the returned result.. Leading with "Structured formatting contains..." is basically repeating the class name.

apjanke · 2017-08-15T02:05:25Z

src/main/java/com/google/maps/model/AutcompleteStructuredFormatting.java

+  /** Contains an array of where the query matched the returned main text. */
+  public AutocompletePrediction.MatchedSubstring mainTextMatchedSubstrings[];
+
+  /** Contains the secondary text of a prediction, usually the location of the place. */


I'd omit "Contains".

apjanke · 2017-08-15T02:12:03Z

Thanks for the interest!

I made some minor style comments.

There's maybe a substantive issue to cover here, too: Are the length and offset values in MatchedSubstring in Unicode characters, or Java chars? Those are often, but not always, the same thing – Java chars are UTF-16 code units, not necessarily full Unicode characters/code points, as characters outside the BMP may be represented by surrogate pairs. Normally in all-Java APIs this isn't an issue, but since we're dealing with an external API here, there may be an "impedance mismatch". I've seen other places in the Google Maps API docs where it describes offsets as being "in Unicode characters". If that's the case here too, it's worth noting. If MatchedSubstring is carrying Unicode-character offsets, instead of char offsets, that might be a surprise to your average Java developer. May be worth noting in the Javadoc which it is.

(I don't know how likely this difference is to crop up in practice for Google Maps, because I don't know how far its localization reaches, but it might be worth noting for full correctness.)

domesticmouse · 2017-08-15T03:00:27Z

The APIs are served in UTF8 I believe, and the length and offsets should be Unicode characters, but I'm unsure whether we document this anywhere.

At this point I'm copying across the documentation from the Places API documentation, as that is what is published, and I'm uncomfortable going above and beyond that.

That said, if someone tests this, and finds that we aren't serving length and offset as unicode characters, then please raise a bug on our issue tracker.

This client library is merely passing through what the APIs return, if the APIs are returning data that is incorrect then we have to fix the APIs themselves, not this client library.

apjanke · 2017-08-15T03:14:43Z

Oh, I don't think it's an issue of correctness with the APIs, just a matter of setting expectations for Java-centric developers who use this library. From what I've seen, most Java APIs seem to use Java char counts for string offsets, and mostly overlook the fact that Java chars are not exactly equivalent to Unicode characters. That is, coders expect offsets and lengths to index in to Strings using the charAt() method or array indexing into the corresponding char[] array, and may need to be given notice that they should be using the offsetByCodePoints() method instead. I think simply noting in the Javadoc for those fields that they are offsets in Unicode characters (as opposed to Java chars) would be sufficient and appropriate. Like the existing documentation for AutocompletePrediction.terms does here; I'd just add "...measured in Unicode characters" to the new length/offset fields' Javadoc.

Sorry if I'm belaboring a point that's already been gone over here in the past.

domesticmouse · 2017-08-15T03:23:26Z

Valid commentary, I'm not totally up to speed on I18N concerns, so I value this feedback. And I'd totally forgotten that Java is UTF16. I've obviously spent too long with Go and it's Unicode runes....

domesticmouse · 2017-08-15T03:24:49Z

How's that look @apjanke?

apjanke · 2017-08-15T03:26:00Z

Yep, LGTM!

domesticmouse · 2017-08-15T03:28:10Z

Thanks for your careful reviews @apjanke!

Adding handling for AutocompletePrediction's structured formatting.

b830072

This is a fix for #244

apjanke suggested changes Aug 15, 2017

View reviewed changes

Cleaning up Javadoc, and fixing class name typo. (Whups)

7459bd2

Adding in unicode characters for MatchedSubstring offset and length.

432c133

apjanke approved these changes Aug 15, 2017

View reviewed changes

domesticmouse merged commit 1cd9db9 into googlemaps:master Aug 15, 2017

Conversation

domesticmouse commented Aug 14, 2017

Uh oh!

domesticmouse commented Aug 14, 2017

Uh oh!

apjanke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apjanke commented Aug 15, 2017

Uh oh!

domesticmouse commented Aug 15, 2017

Uh oh!

apjanke commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domesticmouse commented Aug 15, 2017

Uh oh!

domesticmouse commented Aug 15, 2017

Uh oh!

apjanke commented Aug 15, 2017

Uh oh!

domesticmouse commented Aug 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

apjanke commented Aug 15, 2017 •

edited

Loading