Fixed some code smells. #180
Conversation
…llyEncloses(Location)
| @@ -81,8 +81,8 @@ public static Location forStringLongitudeLatitude(final String locationString) | |||
| { | |||
| throw new CoreException("Invalid Location String: {}", locationString); | |||
| } | |||
| final double latitude = Double.valueOf(split.get(1)); | |||
There was a problem hiding this comment.
You need to be a little careful with using parseDouble functions. They can throw NumberFormatExceptions if the string you are parsing is not valid. And it seems like you don't handle that exception in this function and the one above. I would suggest throwing it from the function.
There was a problem hiding this comment.
This is certainly true. In fact, the original Double.valueOf call had the same problem, if we check out the implementation in java.lang.Double
public static Double valueOf(String s) throws NumberFormatException {
return new Double(parseDouble(s));
}
I am a little hesitant about declaring NumberFormatException to be thrown at the function level, since it is a RuntimeException, and in general we avoid declaring those explicitly in the Atlas codebase (clutters the function signature). However, in this case it may be worth it, since it would be possible for the caller to recover from invalid input.
| @@ -142,7 +141,7 @@ public Rectangle bounds() | |||
| if (this.bounds == null && !this.isEmpty()) | |||
| { | |||
| final Set<Location> locations = new HashSet<>(); | |||
| forEach(polygon -> polygon.forEach(location -> locations.add(location))); | |||
There was a problem hiding this comment.
Nice I really like this type of code style, much easier to read in my opinion.
| @@ -99,11 +99,10 @@ public GeoJsonObject asGeoJson() | |||
| public GeoJsonObject asGeoJsonFeatureCollection() | |||
| { | |||
| final GeoJsonBuilder builder = new GeoJsonBuilder(); | |||
| return builder.createFeatureCollection(Iterables.translate(outers(), outerPolygon -> | |||
| { | |||
There was a problem hiding this comment.
oh common sonar, have mercy, I did this for clarity
There was a problem hiding this comment.
Sonar has spoken. Question not its wisdom. :D
| { | ||
| poorlyFormattedFile.printStackTrace(); | ||
| }); | ||
| .ifPresent(poorlyFormattedFile -> poorlyFormattedFile.printStackTrace()); |
There was a problem hiding this comment.
The continue with the nice style @mgcuthbert referred to above, what about
.ifPresent(StreamCorruptedException::printStackTrace);
Code smells addressed include those listed here:
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5CQl6vpliCeK6yC&open=AWSBg5CQl6vpliCeK6yC
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5CQl6vpliCeK6yD&open=AWSBg5CQl6vpliCeK6yD
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5CQl6vpliCeK6yE&open=AWSBg5CQl6vpliCeK6yE
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5CQl6vpliCeK6yF&open=AWSBg5CQl6vpliCeK6yF
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5CGl6vpliCeK6x3&open=AWSBg5CGl6vpliCeK6x3
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5CGl6vpliCeK6x4&open=AWSBg5CGl6vpliCeK6x4
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5CGl6vpliCeK6xy&open=AWSBg5CGl6vpliCeK6xy
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5Byl6vpliCeK6xZ&open=AWSBg5Byl6vpliCeK6xZ
https://sonarcloud.io/project/issues?id=org.openstreetmap.atlas%3Aatlas&issues=AWSBg5Byl6vpliCeK6xa&open=AWSBg5Byl6vpliCeK6xa