Ignore:
Timestamp:
2020-11-30T10:57:50+01:00 (4 years ago)
Author:
GerdP
Message:

fix issues reported by sonar / coverity, simplify code, add unit tests to improve coverage

File:
1 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java

    r17374 r17376  
    88
    99import java.util.ArrayList;
    10 import java.util.Collections;
    1110import java.util.HashMap;
    1211import java.util.Iterator;
    … …  
    1817import java.util.Set;
    1918import java.util.stream.Collectors;
     19import java.util.stream.Stream;
    2020
    2121import org.openstreetmap.josm.actions.MergeNodesAction;
    … …  
    2525import org.openstreetmap.josm.data.osm.Node;
    2626import org.openstreetmap.josm.data.osm.OsmPrimitive;
    27 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
    2827import org.openstreetmap.josm.data.osm.Storage;
    2928import org.openstreetmap.josm.data.osm.Way;
    … …  
    162161        List<TestError> errors = new ArrayList<>();
    163162
    164         MultiMap<Map<String, String>, OsmPrimitive> mm = new MultiMap<>();
     163        MultiMap<Map<String, String>, Node> mm = new MultiMap<>();
    165164        for (Node n: nodes) {
    166165            mm.put(n.getKeys(), n);
    … …  
    171170        // check whether we have multiple nodes at the same position with the same tag set
    172171        for (Iterator<Map<String, String>> it = mm.keySet().iterator(); it.hasNext();) {
    173             Set<OsmPrimitive> primitives = mm.get(it.next());
     172            Set<Node> primitives = mm.get(it.next());
    174173            if (primitives.size() > 1) {
    175174
    … …  
    178177                }
    179178
    180                 for (OsmPrimitive p : primitives) {
    181                     if (p.getType() == OsmPrimitiveType.NODE) {
    182                         Node n = (Node) p;
    183                         List<OsmPrimitive> lp = n.getReferrers();
    184                         for (OsmPrimitive sp: lp) {
    185                             if (sp.getType() == OsmPrimitiveType.WAY) {
    186                                 boolean typed = false;
    187                                 Way w = (Way) sp;
    188                                 Map<String, String> keys = w.getKeys();
    189                                 for (Entry<String, Boolean> e : typeMap.entrySet()) {
    190                                     if (keys.containsKey(e.getKey())) {
    191                                         e.setValue(Boolean.TRUE);
    192                                         typed = true;
    193                                     }
    194                                 }
    195                                 if (!typed) {
    196                                     typeMap.put("none", Boolean.TRUE);
    197                                 }
     179                for (Node n : primitives) {
     180                    for (Way w: n.getParentWays()) {
     181                        boolean typed = false;
     182                        Map<String, String> keys = w.getKeys();
     183                        for (Entry<String, Boolean> e : typeMap.entrySet()) {
     184                            if (keys.containsKey(e.getKey())) {
     185                                e.setValue(Boolean.TRUE);
     186                                typed = true;
    198187                            }
     188                        }
     189                        if (!typed) {
     190                            typeMap.put("none", Boolean.TRUE);
    199191                        }
    200192                    }
    … …  
    202194
    203195                long nbType = typeMap.entrySet().stream().filter(Entry::getValue).count();
    204 
     196                final TestError.Builder builder;
    205197                if (nbType > 1) {
    206                     errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED)
    207                             .message(tr("Mixed type duplicated nodes"))
    208                             .primitives(primitives)
    209                             .build());
    210                 } else if (typeMap.get(HIGHWAY)) {
    211                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY)
    212                             .message(tr("Highway duplicated nodes"))
    213                             .primitives(primitives)
    214                             .build());
    215                 } else if (typeMap.get(RAILWAY)) {
    216                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY)
    217                             .message(tr("Railway duplicated nodes"))
    218                             .primitives(primitives)
    219                             .build());
    220                 } else if (typeMap.get(WATERWAY)) {
    221                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY)
    222                             .message(tr("Waterway duplicated nodes"))
    223                             .primitives(primitives)
    224                             .build());
    225                 } else if (typeMap.get("boundary")) {
    226                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY)
    227                             .message(tr("Boundary duplicated nodes"))
    228                             .primitives(primitives)
    229                             .build());
    230                 } else if (typeMap.get("power")) {
    231                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER)
    232                             .message(tr("Power duplicated nodes"))
    233                             .primitives(primitives)
    234                             .build());
    235                 } else if (typeMap.get("natural")) {
    236                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL)
    237                             .message(tr("Natural duplicated nodes"))
    238                             .primitives(primitives)
    239                             .build());
    240                 } else if (typeMap.get("building")) {
    241                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING)
    242                             .message(tr("Building duplicated nodes"))
    243                             .primitives(primitives)
    244                             .build());
    245                 } else if (typeMap.get("landuse")) {
    246                     errors.add(TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE)
    247                             .message(tr("Landuse duplicated nodes"))
    248                             .primitives(primitives)
    249                             .build());
     198                    builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_MIXED)
     199                            .message(tr("Mixed type duplicated nodes"));
     200                } else if (Boolean.TRUE.equals(typeMap.get(HIGHWAY))) {
     201                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_HIGHWAY)
     202                            .message(tr("Highway duplicated nodes"));
     203                } else if (Boolean.TRUE.equals(typeMap.get(RAILWAY))) {
     204                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_RAILWAY)
     205                            .message(tr("Railway duplicated nodes"));
     206                } else if (Boolean.TRUE.equals(typeMap.get(WATERWAY))) {
     207                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_WATERWAY)
     208                            .message(tr("Waterway duplicated nodes"));
     209                } else if (Boolean.TRUE.equals(typeMap.get("boundary"))) {
     210                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BOUNDARY)
     211                            .message(tr("Boundary duplicated nodes"));
     212                } else if (Boolean.TRUE.equals(typeMap.get("power"))) {
     213                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_POWER)
     214                            .message(tr("Power duplicated nodes"));
     215                } else if (Boolean.TRUE.equals(typeMap.get("natural"))) {
     216                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_NATURAL)
     217                            .message(tr("Natural duplicated nodes"));
     218                } else if (Boolean.TRUE.equals(typeMap.get("building"))) {
     219                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_BUILDING)
     220                            .message(tr("Building duplicated nodes"));
     221                } else if (Boolean.TRUE.equals(typeMap.get("landuse"))) {
     222                    builder = TestError.builder(parentTest, Severity.ERROR, DUPLICATE_NODE_LANDUSE)
     223                            .message(tr("Landuse duplicated nodes"));
    250224                } else {
    251                     errors.add(TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER)
    252                             .message(tr("Other duplicated nodes"))
    253                             .primitives(primitives)
    254                             .build());
     225                    builder = TestError.builder(parentTest, Severity.WARNING, DUPLICATE_NODE_OTHER)
     226                            .message(tr("Other duplicated nodes"));
    255227                }
     228                errors.add(builder.primitives(primitives).build());
    256229                it.remove();
    257230            }
    … …  
    261234        if (!mm.isEmpty()) {
    262235            List<OsmPrimitive> duplicates = new ArrayList<>();
    263             for (Set<OsmPrimitive> l: mm.values()) {
     236            for (Set<Node> l: mm.values()) {
    264237                duplicates.addAll(l);
    265238            }
    … …  
    278251    public void visit(Node n) {
    279252        if (n.isUsable()) {
    280             if (potentialDuplicates.get(n) == null) {
     253            Object old = potentialDuplicates.get(n);
     254            if (old == null) {
    281255                // in most cases there is just one node at a given position. We
    282256                // avoid to create an extra object and add remember the node
    283257                // itself at this position
    284258                potentialDuplicates.put(n);
    285             } else if (potentialDuplicates.get(n) instanceof Node) {
     259            } else if (old instanceof Node) {
    286260                // we have an additional node at the same position. Create an extra
    287261                // object to keep track of the nodes at this position.
    288262                //
    289                 Node n1 = (Node) potentialDuplicates.get(n);
    290                 List<Node> nodes = new ArrayList<>(2);
    291                 nodes.add(n1);
    292                 nodes.add(n);
    293                 potentialDuplicates.put(nodes);
    294             } else if (potentialDuplicates.get(n) instanceof List<?>) {
    295                 // we have multiple nodes at the same position.
     263                potentialDuplicates.put(Stream.of((Node) old, n).collect(Collectors.toList()));
     264            } else {
     265                // we have more  than two nodes at the same position.
    296266                //
    297                 List<Node> nodes = (List<Node>) potentialDuplicates.get(n);
    298                 nodes.add(n);
     267                ((List<Node>) old).add(n);
    299268            }
    300269        }
    … …  
    307276    @Override
    308277    public Command fixError(TestError testError) {
     278        if (!isFixable(testError))
     279            return null;
    309280        final Set<Node> nodes = testError.primitives(Node.class)
    310281                // Filter nodes that have already been deleted (see #5764 and #5773)
    … …  
    312283                .collect(Collectors.toCollection(LinkedHashSet::new));
    313284
    314         // Merge only if at least 2 nodes remain
    315         if (nodes.size() >= 2) {
    316             // Use first existing node or first node if all nodes are new
    317             Node target = nodes.stream()
    318                     .filter(n -> !n.isNew())
    319                     .findFirst()
    320                     .orElseGet(() -> nodes.iterator().next());
    321 
    322             if (Command.checkOutlyingOrIncompleteOperation(nodes, Collections.singleton(target)) == Command.IS_OK)
    323                 return MergeNodesAction.mergeNodes(nodes, target);
    324         }
    325 
    326         return null; // undoRedo handling done in mergeNodes
     285        // Use first existing node or first node if all nodes are new
     286        Node target = nodes.stream()
     287                .filter(n -> !n.isNew())
     288                .findFirst()
     289                .orElseGet(() -> nodes.iterator().next());
     290
     291        return MergeNodesAction.mergeNodes(nodes, target);
    327292    }
    328293
Note: See TracChangeset for help on using the changeset viewer.