Unconnected way snapper investigation 4975 (#5298)

* UnconnectedWaySnapper formatting and updates.
* DuplicateWayRemover code updates and formatting
* Updated UnconnectedWaySnapper to not mark all ways snapped, only when requested.  Updated unit tests
* Fixing findings from an old PR
* NonContiguousRoadReviewsTest updates with UnconnectedWaySnapper enabled
* NonDestructiveConflationTest updates with UnconnectedWaySnapper enabled
* Formatting
* Fix NonDestructiveConflationTest output
* Fix sonar finding in branch

hoot-core-test/src/test/cpp/hoot/core/ops/UnconnectedWaySnapperTest.cpp

@@ -22,20 +22,20 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2019, 2020, 2021 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2019, 2020, 2021, 2022 Maxar (http://www.maxar.com/)
  */
 
 // Hoot
 #include <hoot/core/TestUtils.h>
 #include <hoot/core/criterion/LinearCriterion.h>
+#include <hoot/core/elements/MapProjector.h>
 #include <hoot/core/elements/OsmMap.h>
 #include <hoot/core/io/OsmMapReaderFactory.h>
 #include <hoot/core/io/OsmMapWriterFactory.h>
 #include <hoot/core/io/OsmXmlReader.h>
 #include <hoot/core/io/OsmXmlWriter.h>
 #include <hoot/core/ops/UnconnectedWaySnapper.h>
 #include <hoot/core/util/ConfigOptions.h>
-#include <hoot/core/elements/MapProjector.h>
 #include <hoot/core/visitors/ElementIdsVisitor.h>
 
 namespace hoot
@@ -58,8 +58,9 @@ class UnconnectedWaySnapperTest : public HootTestFixture
 
 public:
 
-  UnconnectedWaySnapperTest() :
-  HootTestFixture("test-files/ops/UnconnectedWaySnapper/", "test-output/ops/UnconnectedWaySnapper/")
+  UnconnectedWaySnapperTest()
+    : HootTestFixture("test-files/ops/UnconnectedWaySnapper/",
+                      "test-output/ops/UnconnectedWaySnapper/")
   {
     setResetType(ResetEnvironment);
   }

hoot-core/src/main/cpp/hoot/core/algorithms/WayJoiner.cpp

@@ -219,9 +219,9 @@ void WayJoiner::_joinAtNode()
       //  Find all ways connected to this node
       const set<long>& way_ids = nodeToWayMap->getWaysByNode(endpoint_id);
       LOG_VART(way_ids);
-      for (auto wid : way_ids)
+      for (auto child_wid : way_ids)
       {
-        WayPtr child = _map->getWay(wid);
+        WayPtr child = _map->getWay(child_wid);
         if (child && way->getId() != child->getId() && _areJoinable(way, child))
         {
           LOG_VART(child->getElementId());

hoot-core/src/main/cpp/hoot/core/algorithms/alpha-shape/AlphaShape.cpp

@@ -316,8 +316,8 @@ GeometryPtr AlphaShape::toGeometry()
         alpha_values.insert(length);
     }
     //  Get a list of possible alpha values to test
-    for (auto alpha : alpha_values)
-      alpha_options.push_back(static_cast<double>(alpha) / scale);
+    for (auto value : alpha_values)
+      alpha_options.push_back(static_cast<double>(value) / scale);
     //  Iterate the alpha values searching for one that uses at least
     //  90% of the Delauney triangle faces
     bool success = _searchAlpha(alpha, faces, e, preUnionArea, faceCount, alpha_options, 0, alpha_options.size() - 1);

hoot-core/src/main/cpp/hoot/core/algorithms/rubber-sheet/RubberSheet.cpp

@@ -109,9 +109,9 @@ void RubberSheet::setCriteria(const QStringList& criteria, OsmMapPtr map)
   if (!criteria.isEmpty())
   {
     _criteria = std::make_shared<OrCriterion>();
-    for (const auto& crit : qAsConst(criteria))
+    for (const auto& criterion : qAsConst(criteria))
     {
-      const QString critName = crit.trimmed();
+      const QString critName = criterion.trimmed();
       if (!critName.isEmpty())
       {
         LOG_VART(critName);
@@ -753,7 +753,7 @@ void RubberSheet::_writeInterpolator(const std::shared_ptr<const Interpolator>&
   interpolator->writeInterpolator(os);
 }
 
-vector<double> RubberSheet::calculateTiePointDistances()
+vector<double> RubberSheet::calculateTiePointDistances() const
 {
   if (_ties.empty())
     throw HootException("No tie points have been generated.");

hoot-core/src/main/cpp/hoot/core/algorithms/rubber-sheet/RubberSheet.h

@@ -120,7 +120,7 @@ class RubberSheet : public OsmMapOperation, public Configurable, public Conflate
    * @return a collection of distance values
    * @throws HootException if the tie points have not been created
    */
-   std::vector<double> calculateTiePointDistances();
+   std::vector<double> calculateTiePointDistances() const;
 
   void setConflateInfoCache(const std::shared_ptr<ConflateInfoCache>& cache) override
   { _conflateInfoCache = cache; }

hoot-core/src/main/cpp/hoot/core/elements/OsmMap.cpp

@@ -286,11 +286,7 @@ void OsmMap::addNodes(const std::vector<NodePtr>& nodes)
     long minId = nodes[0]->getId();
     long maxId = minId;
 
-    // this seemed like a clever optimization. However, this impacts the BigPertyCmd.sh test b/c
-    // it modifies the order in which the elements are written to the output. Which presumably (?)
-    // impacts the ID when reading the file with re-numbering. Sad.
-    // TODO: The BigPertyCmd.sh test no longer exists, so maybe try it again.
-    size_t minBuckets = _nodes.size() + static_cast<size_t>(nodes.size() * 1.1);
+    size_t minBuckets = _nodes.size() + static_cast<size_t>(static_cast<double>(nodes.size()) * 1.1);
     if (_nodes.bucket_count() < minBuckets)
       _nodes.resize(minBuckets);
 

hoot-core/src/main/cpp/hoot/core/ops/DuplicateWayRemover.cpp

@@ -22,7 +22,7 @@
  * This will properly maintain the copyright information. Maxar
  * copyrights will be updated automatically.
  *
- * @copyright Copyright (C) 2015, 2016, 2017, 2018, 2019, 2020, 2021 Maxar (http://www.maxar.com/)
+ * @copyright Copyright (C) 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022 Maxar (http://www.maxar.com/)
  */
 
 #include "DuplicateWayRemover.h"
@@ -70,13 +70,11 @@ void DuplicateWayRemover::apply(OsmMapPtr& map)
   WayMap wm = _map->getWays();
 
   // go through each way and remove duplicate nodes in one way
-  for (WayMap::const_iterator it = wm.begin(); it != wm.end(); ++it)
+  for (auto it = wm.begin(); it != wm.end(); ++it)
   {
     const WayPtr& w = it->second;
     if (!w)
-    {
       continue;
-    }
     // Since this class operates on elements with generic types, an additional check must be
     // performed here during conflation to enure we don't modify any element not associated with
     // and active conflate matcher in the current conflation configuration.
@@ -90,25 +88,23 @@ void DuplicateWayRemover::apply(OsmMapPtr& map)
     }
     vector<long> newNodes;
     const vector<long>& nodes = w->getNodeIds();
-    for (size_t i = 0; i < nodes.size(); i++)
+    for (auto node_id : nodes)
     {
-      if (newNodes.empty() || newNodes[newNodes.size() - 1] != nodes[i])
-        newNodes.push_back(nodes[i]);
+      if (newNodes.empty() || newNodes[newNodes.size() - 1] != node_id)
+        newNodes.push_back(node_id);
     }
 
     if (newNodes.size() < nodes.size())
       w->setNodes(newNodes);
   }
 
   // go through each way
-  for (WayMap::const_iterator it = wm.begin(); it != wm.end(); ++it)
+  for (auto it = wm.begin(); it != wm.end(); ++it)
   {
     long key = it->first;
     const WayPtr& w = it->second;
     if (!w)
-    {
       continue;
-    }
     else if (_conflateInfoCache &&
              !_conflateInfoCache->elementCanBeConflatedByActiveMatcher(w, className()))
     {
@@ -125,18 +121,17 @@ void DuplicateWayRemover::apply(OsmMapPtr& map)
     // create a map of all the ways that share nodes with this way and the number of nodes shared.
     std::map<long, int> nodesSharedCount;
     const vector<long>& nodes = w->getNodeIds();
-    for (size_t i = 0; i < nodes.size(); i++)
+    for (auto node_id : nodes)
     {
-      const set<long>& ways = n2w[nodes[i]];
-      for (set<long>::iterator wit = ways.begin(); wit != ways.end(); ++wit)
+      const set<long>& ways = n2w[node_id];
+      for (auto way_id : ways)
       {
-        if (*wit != w->getId() && _isCandidateWay(w))
-          nodesSharedCount[*wit]++;
+        if (way_id != w->getId() && _isCandidateWay(w))
+          nodesSharedCount[way_id]++;
       }
     }
 
-    for (std::map<long, int>::iterator wit = nodesSharedCount.begin();
-         wit != nodesSharedCount.end(); ++wit)
+    for (auto wit = nodesSharedCount.begin(); wit != nodesSharedCount.end(); ++wit)
     {
       // If a way shares 2 or more nodes,
       if (wit->second >= 2 && _map->containsWay(wit->first) && _map->containsWay(w->getId()))
@@ -178,9 +173,9 @@ bool DuplicateWayRemover::_isCandidateWay(const ConstWayPtr& w) const
 {
   return
     // is this a linear way
-    (LinearCriterion().isSatisfied(w) &&
-     // if this is not part of a relation
-     _map->getIndex().getParents(w->getElementId()).empty());
+    LinearCriterion().isSatisfied(w) &&
+    // if this is not part of a relation
+    _map->getIndex().getParents(w->getElementId()).empty();
 }
 
 void DuplicateWayRemover::_splitDuplicateWays(WayPtr w1, WayPtr w2, bool rev1, bool rev2)
@@ -198,7 +193,7 @@ void DuplicateWayRemover::_splitDuplicateWays(WayPtr w1, WayPtr w2, bool rev1, b
     // _splitDuplicateWays is always called where num_nodes(w1) >= num_nodes(w2), so the following
     // logic works
     if (nodes1.size() == nodes2.size() &&
-        nodes1.size() == static_cast<vector<long>::size_type>(length))
+        nodes1.size() == static_cast<size_t>(length))
     {
       //  Merge the two ways' tags
       w1->setTags(mergedTags);
@@ -210,7 +205,7 @@ void DuplicateWayRemover::_splitDuplicateWays(WayPtr w1, WayPtr w2, bool rev1, b
       //  Remove w2
       _replaceMultiple(w2, vector<WayPtr>());
     }
-    else if (nodes2.size() == static_cast<vector<long>::size_type>(length))
+    else if (nodes2.size() == static_cast<size_t>(length))
     {
       //  Way 2 is completely engulfed in way 1, shrink way 1 and leave way 2 intact
       vector<WayPtr> ways1 = _splitWay(w1, lcs.getW1Index(), length, false);
@@ -346,16 +341,16 @@ void DuplicateWayRemover::_replaceMultiple(
 
   vector<RelationData::Entry> newValues;
   newValues.reserve(ways.size());
-  for (vector<WayPtr>::size_type i = 0; i < ways.size(); ++i)
+  for (size_t i = 0; i < ways.size(); ++i)
     newValues[i].setElementId(ways[i]->getElementId());
 
   RelationData::Entry old(oldWay->getElementId());
 
   //  Make a copy just in case the index changes while we're replacing elements.
   set<long> rids = _map->getIndex().getElementToRelationMap()->getRelationByElement(oldWay.get());
-  for (set<long>::const_iterator it = rids.begin(); it != rids.end(); ++it)
+  for (auto relation_id : rids)
   {
-    const RelationPtr& r = _map->getRelation(*it);
+    const RelationPtr& r = _map->getRelation(relation_id);
     if (r)
       r->replaceElements(old, newValues.begin(), newValues.end());
   }