Fixing code smells (#181)

* Turn Restriction code smells * ComplexIslandFinder code smells * Fix MultiAtlasBorderFixer code smells * Fixing ChangeSet code smells * RawAtlasGenerator code smells * Fixing Way Section Processor code smells * Fixing Raw Atlas Relation Slicer * Fixing Country Boundary Compare and Map code smells * Fixing Country Shard Listing code smells * Fixing Coordinate converters code smells * Fixing Crawler code smells * Cleaning up Polygon code smells * Resolving Polygon merge conflict * Actually fixing the conflcit * One more time
osmlab · Jul 27, 2018 · 736b649 · 736b649
1 parent 16916b4
commit 736b649
Show file tree

Hide file tree

Showing 15 changed files with 151 additions and 173 deletions.
diff --git a/src/main/java/org/openstreetmap/atlas/geography/Polygon.java b/src/main/java/org/openstreetmap/atlas/geography/Polygon.java
@@ -347,8 +347,8 @@ public boolean isApproximatelyNSided(final int expectedNumberOfSides, final Angl
         {
             final Optional<Heading> nextHeading = segments.get(segmentIndex++).heading();
 
-            // If heading difference is greater than threshold, then increment heading change
-            // counter and update previous heading, which is used as reference
+            // If heading difference is greater than threshold, then increment heading
+            // change counter and update previous heading, which is used as reference
             if (nextHeading.isPresent()
                     && previousHeading.get().difference(nextHeading.get()).isGreaterThan(threshold))
             {
@@ -561,7 +561,6 @@ public List<Polygon> triangles()
     {
         final ConformingDelaunayTriangulationBuilder trianguler = new ConformingDelaunayTriangulationBuilder();
         // Populate the delaunay triangulation builder
-        // trianguler.setSites(Iterables.asList(JTS_POLYGON_CONVERTER.convert(this).getCoordinates()));
         trianguler.setSites(JTS_POLYGON_CONVERTER.convert(this));
         final GeometryCollection triangleCollection = (GeometryCollection) trianguler
                 .getTriangles(JtsPrecisionManager.getGeometryFactory());

diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/Crawler.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/Crawler.java
@@ -35,7 +35,7 @@ protected int onRun(final CommandMap command)
         initialize(command);
         if (inputFolder != null)
         {
-            this.logger.info("Loading Atlas from " + inputFolder);
+            this.logger.info("Loading Atlas from {}", inputFolder);
             final Atlas atlas = loadAtlas(command);
             processAtlas(atlasName, atlas, outputFolder.getPath());
         }

diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/items/TurnRestriction.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/items/TurnRestriction.java
@@ -46,12 +46,12 @@ public enum TurnRestrictionType
 
     private static final long serialVersionUID = 7043701090121113526L;
 
-    private final Route from;
+    private Route from;
     private final Relation relation;
     private Route route;
-    private final Route too;
-    private final TurnRestrictionType type;
-    private final Route via;
+    private Route too;
+    private TurnRestrictionType type;
+    private Route via;
 
     /**
      * Create a {@link TurnRestriction} from a {@link Relation}
@@ -74,6 +74,22 @@ public static Optional<TurnRestriction> from(final Relation relation)
         }
     }
 
+    public static TurnRestrictionType getTurnRestrictionType(final Relation relation)
+    {
+        if (TurnRestrictionTag.isNoPathRestriction(relation))
+        {
+            return TurnRestrictionType.NO;
+        }
+        else if (TurnRestrictionTag.isOnlyPathRestriction(relation))
+        {
+            return TurnRestrictionType.ONLY;
+        }
+        else
+        {
+            return TurnRestrictionType.OTHER;
+        }
+    }
+
     /**
      * Test if a {@link Route} is restricted.
      *
@@ -209,22 +225,22 @@ private static boolean routeContainsAllTurnRestrictionParts(
             final TurnRestriction turnRestriction, final Route route)
     {
         final Optional<Route> possibleVia = turnRestriction.getVia();
-        final boolean viaMatches = possibleVia.isPresent() ? route.isSubRoute(possibleVia.get())
-                : true;
+        boolean viaMatches = true;
+        if (possibleVia.isPresent())
+        {
+            viaMatches = route.isSubRoute(possibleVia.get());
+        }
         return viaMatches && route.isSubRoute(turnRestriction.getTo())
                 && route.isSubRoute(turnRestriction.getFrom());
     }
 
     private TurnRestriction(final Relation relation)
     {
-        // to -> too for checkstyle.
-        Route from = null;
-        Route via = null;
-        Route too = null;
+        Route fromMember = null;
+        Route viaMember = null;
+        Route toMember = null;
         this.relation = relation;
-        this.type = TurnRestrictionTag.isNoPathRestriction(relation) ? TurnRestrictionType.NO
-                : TurnRestrictionTag.isOnlyPathRestriction(relation) ? TurnRestrictionType.ONLY
-                        : TurnRestrictionType.OTHER;
+        this.type = getTurnRestrictionType(relation);
         if (this.type == TurnRestrictionType.OTHER)
         {
             this.from = null;
@@ -238,8 +254,7 @@ private TurnRestriction(final Relation relation)
             {
                 throw new CoreException("Relation {} is not a restriction.", relation);
             }
-            // Try to re-build the route, based on the "from", "via" and "to" members of the
-            // relation
+            // Try to re-build the route, based on the "from", "via" and "to" members
 
             // Build the via members
             final Set<AtlasItem> viaMembers = relation.members().stream()
@@ -271,7 +286,8 @@ private TurnRestriction(final Relation relation)
                 if (isSameRoadViaAndTo(relation))
                 {
                     throw new CoreException(
-                            "This relation has same members in from and to, but has no via members to disambiguate.");
+                            "Relation {} has same members in from and to, but has no via members to disambiguate.",
+                            relation.getIdentifier());
                 }
                 relation.members().stream().filter(
                         member -> member.getRole().equals(RelationTypeTag.RESTRICTION_ROLE_TO))
@@ -281,29 +297,26 @@ private TurnRestriction(final Relation relation)
             // Filter the members to extract only the "from" members that are connected at the end
             // to via members if any, or to the start of "to" members.
             final Set<Edge> fromMembers = new TreeSet<>();
-            relation.members().stream().filter(member ->
-            {
-                return member.getRole().equals(RelationTypeTag.RESTRICTION_ROLE_FROM)
-                        && member.getEntity() instanceof Edge
-                        && (!viaMembers.isEmpty()
-                                && ((Edge) member.getEntity()).isConnectedAtEndTo(viaMembers)
-                                || ((Edge) member.getEntity())
-                                        .isConnectedAtEndTo(temporaryToMembers));
-            }).forEach(member -> fromMembers.add((Edge) member.getEntity()));
-            from = Route.fromNonArrangedEdgeSet(fromMembers, false);
+            relation.members().stream().filter(member -> member.getRole()
+                    .equals(RelationTypeTag.RESTRICTION_ROLE_FROM)
+                    && member.getEntity() instanceof Edge
+                    && (!viaMembers.isEmpty()
+                            && ((Edge) member.getEntity()).isConnectedAtEndTo(viaMembers)
+                            || ((Edge) member.getEntity()).isConnectedAtEndTo(temporaryToMembers)))
+                    .forEach(member -> fromMembers.add((Edge) member.getEntity()));
+            fromMember = Route.fromNonArrangedEdgeSet(fromMembers, false);
 
             // Filter the members to extract only the "to" members that are connected at the
             // beginning to via members if any, or to the end of "from" members.
             final Set<Edge> toMembers = new TreeSet<>();
-            relation.members().stream().filter(member ->
-            {
-                return member.getRole().equals(RelationTypeTag.RESTRICTION_ROLE_TO)
-                        && member.getEntity() instanceof Edge
-                        && (!viaMembers.isEmpty()
-                                && ((Edge) member.getEntity()).isConnectedAtStartTo(viaMembers)
-                                || ((Edge) member.getEntity()).isConnectedAtStartTo(fromMembers));
-            }).forEach(member -> toMembers.add((Edge) member.getEntity()));
-            too = Route.fromNonArrangedEdgeSet(toMembers, false);
+            relation.members().stream().filter(member -> member.getRole()
+                    .equals(RelationTypeTag.RESTRICTION_ROLE_TO)
+                    && member.getEntity() instanceof Edge
+                    && (!viaMembers.isEmpty()
+                            && ((Edge) member.getEntity()).isConnectedAtStartTo(viaMembers)
+                            || ((Edge) member.getEntity()).isConnectedAtStartTo(fromMembers)))
+                    .forEach(member -> toMembers.add((Edge) member.getEntity()));
+            toMember = Route.fromNonArrangedEdgeSet(toMembers, false);
 
             // Take only the via members that are edges. Build this last to guarantee a route from
             // from to too.
@@ -315,22 +328,23 @@ private TurnRestriction(final Relation relation)
                 // It's possible that the via edge is bi-directional. To prevent both edges from
                 // being put into the route, build a route using all unique edges once. This method
                 // attempts to build a route from from the end of from to start of too.
-                via = Route.buildFullRouteIgnoringReverseEdges(viaEdges, from.end().end(),
-                        too.start().start());
+                viaMember = Route.buildFullRouteIgnoringReverseEdges(viaEdges,
+                        this.from.end().end(), this.too.start().start());
             }
         }
         catch (final CoreException e)
         {
             logger.trace("Could not build TurnRestriction from relation {}", relation, e);
-            from = null;
-            via = null;
-            too = null;
+            fromMember = null;
+            viaMember = null;
+            toMember = null;
         }
-        this.from = from;
-        this.via = via;
-        this.too = too;
+        this.from = fromMember;
+        this.via = viaMember;
+        this.too = toMember;
     }
 
+    @SuppressWarnings("deprecation")
     public LocationIterableProperties asGeoJson()
     {
         final Map<String, String> tagsNo = Maps.hashMap("highway", "primary", "oneway", "yes",
@@ -471,7 +485,6 @@ private boolean isSameRoadViaAndTo(final Relation relation)
 
     private boolean isValid()
     {
-        final boolean valid = this.from != null && this.too != null && route() != null;
-        return valid;
+        return this.from != null && this.too != null && route() != null;
     }
 }
diff --git a/...va/org/openstreetmap/atlas/geography/atlas/items/complex/islands/ComplexIslandFinder.java b/...va/org/openstreetmap/atlas/geography/atlas/items/complex/islands/ComplexIslandFinder.java
@@ -25,8 +25,6 @@ public Iterable<ComplexIsland> find(final Atlas atlas)
         final Iterable<Relation> relations = atlas.relations(
                 relation -> relation.isMultiPolygon() && relation.hasMultiPolygonMembers(Ring.INNER)
                         && (isLake(relation) || isRiver(relation) || isReservoir(relation)));
-        final Iterable<ComplexIsland> complexEntities = Iterables.translate(relations,
-                ComplexIsland::new);
-        return complexEntities;
+        return Iterables.translate(relations, ComplexIsland::new);
     }
 }
diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/multi/MultiAtlasBorderFixer.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/multi/MultiAtlasBorderFixer.java
@@ -45,6 +45,8 @@ public class MultiAtlasBorderFixer implements Serializable
     private static final long serialVersionUID = -3774372864489402091L;
     private static final Logger logger = LoggerFactory.getLogger(MultiAtlasBorderFixer.class);
 
+    private static final String MISSING_FIX_ATLAS = "Fix Atlas is not present.";
+
     // Keeps track of whether border fix process is completed or not
     private boolean isCompleted;
 
@@ -57,7 +59,7 @@ public class MultiAtlasBorderFixer implements Serializable
 
     // Set of fixed country OSM identifiers
     private final Set<Long> fixedCountryOsmIdentifiers;
-    private Optional<Atlas> fixAtlas;
+    private transient Optional<Atlas> fixAtlas;
     private final List<Atlas> subAtlases;
     private final HashSet<Long> countryOsmIdentifierWithReverseEdges;
     private final MultiMap<Long, Long> countryOsmIdentifierToEdgeIdentifiers;
@@ -269,13 +271,9 @@ protected void fixBorderIssues()
 
                 // perform a set union instead of wiping out the set that is already mapped at the
                 // current identifier
-                candidateRelationMembers.forEach((identifier, temporaryRelationMember) ->
-                {
-                    temporaryRelationMember.forEach(member ->
-                    {
-                        relationMembersToUpdate.add(identifier, member);
-                    });
-                });
+                candidateRelationMembers.forEach(
+                        (identifier, temporaryRelationMember) -> temporaryRelationMember.forEach(
+                                member -> relationMembersToUpdate.add(identifier, member)));
 
                 // Mark old edge nodes/relations to be ignored
                 markItemsToBeIgnored(roads, hasReverseEdges);
@@ -303,7 +301,7 @@ protected void fixBorderIssues()
                         .get(relation.getIdentifier());
                 if (members != null && !members.isEmpty())
                 {
-                    members.forEach(member -> relation.addMember(member));
+                    members.forEach(relation::addMember);
                 }
                 else
                 {
@@ -328,25 +326,25 @@ protected void fixBorderIssues()
 
     protected Edge fixEdge(final long identifier)
     {
-        return this.fixAtlas.orElseThrow(() -> new CoreException("Fix Atlas is not present."))
+        return this.fixAtlas.orElseThrow(() -> new CoreException(MISSING_FIX_ATLAS))
                 .edge(identifier);
     }
 
     protected Node fixNode(final long identifier)
     {
-        return this.fixAtlas.orElseThrow(() -> new CoreException("Fix Atlas is not present."))
+        return this.fixAtlas.orElseThrow(() -> new CoreException(MISSING_FIX_ATLAS))
                 .node(identifier);
     }
 
     protected Relation fixRelation(final Long identifier)
     {
-        return this.fixAtlas.orElseThrow(() -> new CoreException("Fix Atlas is not present."))
+        return this.fixAtlas.orElseThrow(() -> new CoreException(MISSING_FIX_ATLAS))
                 .relation(identifier);
     }
 
     protected Atlas getFixAtlas()
     {
-        return this.fixAtlas.orElseThrow(() -> new CoreException("Fix Atlas is not present."));
+        return this.fixAtlas.orElseThrow(() -> new CoreException(MISSING_FIX_ATLAS));
     }
 
     protected MultiMapWithSet<Long, Long> getNodeIdentifiersToRemovedInEdges()
@@ -468,10 +466,10 @@ private Optional<Atlas> applyFixesToAtlas(final Map<Long, Node> newNodes,
         }
 
         // Get fixed atlas
-        final Atlas fixAtlas = fixBuilder.get();
-        logger.debug("Fix atlas meta data: {}", fixAtlas.metaData());
+        final Atlas fixedAtlas = fixBuilder.get();
+        logger.debug("Fix atlas meta data: {}", fixedAtlas.metaData());
 
-        return Optional.of(fixAtlas);
+        return Optional.of(fixedAtlas);
     }
 
     /**
@@ -542,7 +540,7 @@ private MultiMapWithSet<Long, String> collectRoles(
                         final String role = member.getRole();
                         roles.add(relationIdentifier, role);
                     }
-                    catch (final Throwable error)
+                    catch (final Exception error)
                     {
                         throw new CoreException("Error adding in roles: {}", relation, error);
                     }

diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/pbf/slicing/ChangeSet.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/pbf/slicing/ChangeSet.java
@@ -12,6 +12,8 @@
 import org.openstreetmap.osmosis.core.domain.v0_6.Node;
 import org.openstreetmap.osmosis.core.domain.v0_6.Relation;
 import org.openstreetmap.osmosis.core.domain.v0_6.Way;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * This class stores all changes made during data processing. Until now all changes falls into the
@@ -21,6 +23,8 @@
  */
 public class ChangeSet
 {
+    private static final Logger logger = LoggerFactory.getLogger(ChangeSet.class);
+
     private final List<Node> createdNodes;
     private final List<Node> deletedNodes;
     private final List<Way> createdWays;
@@ -194,7 +198,7 @@ public void save(final OutputStream output)
         }
         catch (final IOException e)
         {
-            e.printStackTrace();
+            logger.error("Could not save ChangeSet", e);
         }
     }