Skip to content

Fix static marker annotation url generation #479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Guardiola31337 merged 1 commit into from
Jun 17, 2017
Merged

Fix static marker annotation url generation #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 47 additions & 6 deletions ...s/src/main/java/com/mapbox/services/api/staticimage/v1/models/StaticMarkerAnnotation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public StaticMarkerAnnotation(Builder builder) {
TextUtils.formatCoordinate(builder.getLat(), builder.getPrecision())
);
} else {
marker = String.format(Constants.DEFAULT_LOCALE, "%s-%s+%s(%s,%s)",
marker = String.format(Constants.DEFAULT_LOCALE, "%s%s%s(%s,%s)",
builder.getName(),
builder.getLabel(),
builder.getColor(),
Expand All @@ -58,7 +58,7 @@ public StaticMarkerAnnotation(Builder builder) {
marker = String.format(Constants.DEFAULT_LOCALE, "url-%s(%s,%s)",
builder.getUrl(), builder.getLon(), builder.getLat());
} else {
marker = String.format(Constants.DEFAULT_LOCALE, "%s-%s+%s(%f,%f)", builder.getName(),
marker = String.format(Constants.DEFAULT_LOCALE, "%s%s%s(%f,%f)", builder.getName(),
builder.getLabel(), builder.getColor(), builder.getLon(), builder.getLat());
}
}
Expand All @@ -81,6 +81,11 @@ public String getMarker() {
*/
public static class Builder {

private static final String EMPTY = "";
private static final String HYPHEN_CHAR = "-";
private static final String PLUS_CHAR = "+";
private static final String NUMERIC_FROM_ZERO_TO_NINETYNINE_REGEX = "^(0|[1-9][0-9]{0,1})$";
private static final String ONE_ALPHABET_LETTER_REGEX = "^([a-zA-Z])$";
private String name;
private String label = "";
private String color = "";
Expand Down Expand Up @@ -144,12 +149,17 @@ public Builder setName(String name) {
* @since 2.1.0
*/
public String getLabel() {
return label;
if (label.isEmpty()) {
return EMPTY;
Copy link

@cammace cammace Jun 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return null if a label isn't set?

Copy link
Contributor Author

@Guardiola31337 Guardiola31337 Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace null is not valid because getLabel is called when parsing (String.format) and label is an optional param (note that -was removed from the default string because if the label was empty url was incorrect. A Null Object pattern could be implemented though.

BTW null is considered The worst mistake of computer science: Null References: The Billion Dollar Mistake 😉

Copy link

@cammace cammace Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, I've read that interview 😄 . Sounds good.

}

return HYPHEN_CHAR.concat(label).toLowerCase();
}

/**
* StaticMarkerAnnotation symbol. Options are an alphanumeric label {@code a} through {@code z}, {@code 0} through
* {@code 99}, or a valid Maki icon. If a letter is requested, it will be rendered uppercase only.
* StaticMarkerAnnotation symbol. Options are an alphanumeric label {@code a}-{@code A} through
* {@code z}-{@code Z}, {@code 0} through {@code 99}, or a valid Maki icon. If a letter is requested, it will be
* rendered uppercase only.
*
* @param label String containing the marker symbol.
* @return This StaticMarkerAnnotation builder.
Expand All @@ -167,7 +177,11 @@ public Builder setLabel(String label) {
* @since 2.1.0
*/
public String getColor() {
return color;
if (color.isEmpty()) {
return EMPTY;
Copy link

@cammace cammace Jun 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return null if a color isn't set?

Copy link
Contributor Author

@Guardiola31337 Guardiola31337 Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

}

return PLUS_CHAR.concat(color);
}

/**
Expand Down Expand Up @@ -296,6 +310,33 @@ public StaticMarkerAnnotation build() throws ServicesException {
}
}

if (!label.isEmpty()) {
boolean isANumber = true;
try {
Integer.parseInt(label);
} catch (NumberFormatException notANumber) {
isANumber = false;
}
if (isANumber) {
Pattern pattern = Pattern.compile(NUMERIC_FROM_ZERO_TO_NINETYNINE_REGEX);
Matcher matcher = pattern.matcher(label);
if (!matcher.matches()) {
throw new ServicesException("You need to pass an alphanumeric label [0-99] code.");
}
} else {
// TODO Find a better way to know when a label is not a valid Maki icon
// Right now, this is not verified
// At the moment there's no Maki icon name with 2 characters or less
if (label.length() < 3) {
Pattern pattern = Pattern.compile(ONE_ALPHABET_LETTER_REGEX);
Matcher matcher = pattern.matcher(label);
if (!matcher.matches()) {
throw new ServicesException("You need to pass an alphanumeric label [a-zA-Z] code.");
}
}
}
}

return new StaticMarkerAnnotation(this);
}
}
Expand Down
100 changes: 96 additions & 4 deletions ...ices/src/test/java/com/mapbox/services/api/staticimage/v1/StaticMarkerAnnotationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void testSanity() throws ServicesException {
.setName(Constants.PIN_SMALL)
.build();

assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s-+(1.000000,2.000000)"));
assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s(1.000000,2.000000)"));
}

@Test
Expand Down Expand Up @@ -65,8 +65,76 @@ public void requireMarkerLon() throws ServicesException {
new StaticMarkerAnnotation.Builder().setName(Constants.PIN_SMALL).setLat(2.0).build();
}

@Test(expected = ServicesException.class)
Copy link

@cammace cammace Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be more explicit on which ServiceException we expect to occur here?

Copy link
Contributor Author

@Guardiola31337 Guardiola31337 Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO just testing that an exception is thrown when an url is not valid is 👌
If more explicitness needed, what about creating a new specific exception?
BTW, is that explicitness really needed in this case?

public void requiresOneAlphabetCharLabelIfPresent() throws ServicesException {
new StaticMarkerAnnotation.Builder()
.setName(Constants.PIN_SMALL)
.setLat(2.0)
.setLon(2.0)
.setLabel("aa")
.build();
}

@Test(expected = ServicesException.class)
public void requiresZeroToNinetynineNumericCharLabelIfPresent() throws ServicesException {
new StaticMarkerAnnotation.Builder()
.setName(Constants.PIN_SMALL)
.setLat(2.0)
.setLon(2.0)
.setLabel("100")
.build();
}

@Test
public void checksMarkerAlphabetCharLabel() throws ServicesException {
StaticMarkerAnnotation staticMarkerAnnotation = new StaticMarkerAnnotation.Builder()
.setLat(2.0)
.setLon(1.0)
.setName(Constants.PIN_SMALL)
.setLabel("a")
.build();

assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s-a(1.000000,2.000000)"));
}

@Test
public void checksMarkerCaseInsensitiveAlphabetCharLabel() throws ServicesException {
StaticMarkerAnnotation staticMarkerAnnotation = new StaticMarkerAnnotation.Builder()
.setLat(2.0)
.setLon(1.0)
.setName(Constants.PIN_SMALL)
.setLabel("A")
.build();

assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s-a(1.000000,2.000000)"));
}

@Test
public void checksMarkerNumericCharLabel() throws ServicesException {
StaticMarkerAnnotation staticMarkerAnnotation = new StaticMarkerAnnotation.Builder()
.setLat(2.0)
.setLon(1.0)
.setName(Constants.PIN_SMALL)
.setLabel("33")
.build();

assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s-33(1.000000,2.000000)"));
}

@Test
public void requireValidMarkerColor() throws ServicesException {
public void checksMarkerMapboxMakiIconCharLabel() throws ServicesException {
StaticMarkerAnnotation staticMarkerAnnotation = new StaticMarkerAnnotation.Builder()
.setLat(2.0)
.setLon(1.0)
.setName(Constants.PIN_SMALL)
.setLabel("bakery")
.build();

assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s-bakery(1.000000,2.000000)"));
}

@Test
public void requiresValidColorCodeIfPresent() throws ServicesException {
thrown.expect(ServicesException.class);
thrown.expectMessage(startsWith("You need to pass 3- or 6-digit hexadecimal color code."));

Expand All @@ -78,6 +146,30 @@ public void requireValidMarkerColor() throws ServicesException {
.build();
}

@Test
public void checksMarkerWithThreeDigitColorCode() throws ServicesException {
StaticMarkerAnnotation staticMarkerAnnotation = new StaticMarkerAnnotation.Builder()
.setLat(2.0)
.setLon(1.0)
.setName(Constants.PIN_SMALL)
.setColor("333")
.build();

assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s+333(1.000000,2.000000)"));
}

@Test
public void checksMarkerWithSixDigitHexColorCode() throws ServicesException {
StaticMarkerAnnotation staticMarkerAnnotation = new StaticMarkerAnnotation.Builder()
.setLat(2.0)
.setLon(1.0)
.setName(Constants.PIN_SMALL)
.setColor("666666")
.build();

assertTrue(staticMarkerAnnotation.getMarker().contains("pin-s+666666(1.000000,2.000000)"));
}

@Test
public void markerPositionWorking() throws ServicesException {
Position position = Position.fromCoordinates(1.0, 2.0);
Expand All @@ -102,7 +194,7 @@ public void singleMarkerInUrl() {
.setWidth(100).setHeight(200)
.build();

assertTrue(image.getUrl().toString().contains("pin-s-+(1.000000,2.000000)"));
assertTrue(image.getUrl().toString().contains("pin-s(1.000000,2.000000)"));
}

@Test
Expand All @@ -125,7 +217,7 @@ public void doubleMarkersInUrl() {
.setWidth(100).setHeight(200)
.build();

assertTrue(image.getUrl().toString().contains("pin-s-+(1.000000,2.000000),pin-m-+(5.000000,6.000000)"));
assertTrue(image.getUrl().toString().contains("pin-s(1.000000,2.000000),pin-m(5.000000,6.000000)"));
}

@Test
Expand Down