Fix static marker annotation url generation #479
Conversation
| @@ -167,7 +177,11 @@ public Builder setLabel(String label) { | |||
| * @since 2.1.0 | |||
| */ | |||
| public String getColor() { | |||
| return color; | |||
| if (color.isEmpty()) { | |||
| return EMPTY; | |||
There was a problem hiding this comment.
Why not just return null if a color isn't set?
| @@ -144,12 +149,17 @@ public Builder setName(String name) { | |||
| * @since 2.1.0 | |||
| */ | |||
| public String getLabel() { | |||
| return label; | |||
| if (label.isEmpty()) { | |||
| return EMPTY; | |||
There was a problem hiding this comment.
Why not just return null if a label isn't set?
There was a problem hiding this comment.
@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 😉
There was a problem hiding this comment.
lol, I've read that interview 😄 . Sounds good.
| @@ -65,8 +65,76 @@ public void requireMarkerLon() throws ServicesException { | |||
| new StaticMarkerAnnotation.Builder().setName(Constants.PIN_SMALL).setLat(2.0).build(); | |||
| } | |||
|
|
|||
| @Test(expected = ServicesException.class) | |||
There was a problem hiding this comment.
Should we be more explicit on which ServiceException we expect to occur here?
There was a problem hiding this comment.
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?
Fixes mapbox/mapbox-gl-native#9107
👀 @zugaldia @cammace