Changeset 36181 in osm

Timestamp:

2023-10-25T18:19:52+02:00 (14 months ago)

Author:

taylor.smock

Message:

Fix #6855: Reverse terracer considers buildings without an address when reversing a terrace

This also fixes several lint issues.

Location:

applications/editors/josm/plugins/terracer

Files:

• src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputDialog.java (modified) (15 diffs)
• src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputHandler.java (modified) (16 diffs)
• src/org/openstreetmap/josm/plugins/terracer/ReverseTerraceAction.java (modified) (9 diffs)
• src/org/openstreetmap/josm/plugins/terracer/TerracerAction.java (modified) (32 diffs)
• src/org/openstreetmap/josm/plugins/terracer/TerracerPlugin.java (modified) (2 diffs)
• test (added)
• test/unit (added)
• test/unit/org (added)
• test/unit/org/openstreetmap (added)
• test/unit/org/openstreetmap/josm (added)
• test/unit/org/openstreetmap/josm/plugins (added)
• test/unit/org/openstreetmap/josm/plugins/terracer (added)
• test/unit/org/openstreetmap/josm/plugins/terracer/ReverseTerraceActionTest.java (added)

applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputDialog.java

@@ -4 +4 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 
-import java.awt.Choice;
 import java.awt.Color;
 import java.awt.Container;
@@ -11 +10 @@
 import java.awt.GridBagLayout;
 import java.awt.event.ActionEvent;
-import java.util.ArrayList;
 import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
 import java.util.TreeSet;
 
 import javax.swing.BoxLayout;
 import javax.swing.JCheckBox;
+import javax.swing.JComponent;
 import javax.swing.JLabel;
 import javax.swing.JPanel;
@@ -32 +33 @@
 import org.openstreetmap.josm.gui.tagging.ac.AutoCompletionManager;
 import org.openstreetmap.josm.gui.util.WindowGeometry;
+import org.openstreetmap.josm.gui.widgets.JosmComboBox;
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.tools.GBC;
@@ -37 +39 @@
 /**
  * The HouseNumberInputDialog is the layout of the house number input logic.
- *
+ * <p>
  *  This dialog is concerned with the layout, all logic goes into the
  *  HouseNumberinputHandler class.
@@ -58 +60 @@
     private final String buildingType;
     private final boolean relationExists;
-    final ArrayList<Node> housenumbers;
+    final List<Node> houseNumbers;
 
     protected static final String DEFAULT_MESSAGE = tr("Enter housenumbers or amount of segments");
     private Container jContentPane;
     private JPanel inputPanel;
-    private JLabel loLabel;
     JTextField lo;
-    private JLabel hiLabel;
     JTextField hi;
     private JLabel numbersLabel;
     JTextField numbers;
-    private JLabel streetLabel;
     AutoCompComboBox<String> streetComboBox;
-    private JLabel buildingLabel;
     AutoCompComboBox<AutoCompletionItem> buildingComboBox;
-    private JLabel segmentsLabel;
     JTextField segments;
     JTextArea messageLabel;
-    private JLabel interpolationLabel;
-    Choice interpolation;
+    JosmComboBox<String> interpolationType;
     JCheckBox handleRelationCheckBox;
     JCheckBox keepOutlineCheckBox;
@@ -84 +80 @@
 
     /**
+     * Create a new dialog to get settings for the current operation
      * @param street If street is not null, we assume, the name of the street to be fixed
      * and just show a label. If street is null, we show a ComboBox/InputField.
@@ -91 +88 @@
      * @param buildingType The value to add for building key
      * @param relationExists If the buildings can be added to an existing relation or not.
-     * @param housenumbers a list of house numbers in this outline (may be empty)
+     * @param houseNumbers a list of house numbers in this outline (may be empty)
+     * @param handler The callback for the inputs
      */
     public HouseNumberInputDialog(HouseNumberInputHandler handler, Way street, String streetName,
-            String buildingType, boolean relationExists, ArrayList<Node> housenumbers) {
+            String buildingType, boolean relationExists, List<Node> houseNumbers) {
         super(MainApplication.getMainFrame(),
                 tr("Terrace a house"),
@@ -105 +103 @@
         this.buildingType = buildingType;
         this.relationExists = relationExists;
-        this.housenumbers = housenumbers;
+        this.houseNumbers = houseNumbers;
         handler.dialog = this;
         JPanel content = getInputPanel();
         setContent(content);
-        setButtonIcons(new String[] {"ok", "cancel" });
+        setButtonIcons("ok", "cancel");
         getJContentPane();
         initialize();
@@ -128 +126 @@
         this.hi.addFocusListener(this.inputHandler);
         this.segments.addFocusListener(this.inputHandler);
-        this.interpolation.addItemListener(this.inputHandler);
+        this.interpolationType.addItemListener(this.inputHandler);
     }
 
@@ -165 +163 @@
             messageLabel.setFocusable(false); // Needed so that lowest number can have focus immediately
 
-            interpolationLabel = new JLabel(tr("Interpolation"));
-            segmentsLabel = new JLabel(tr("Segments"));
-            streetLabel = new JLabel(tr("Street"));
-            buildingLabel = new JLabel(tr("Building"));
-            loLabel = new JLabel(tr("Lowest Number"));
+            JLabel interpolationLabel = new JLabel(tr("Interpolation"));
+            JLabel segmentsLabel = new JLabel(tr("Segments"));
+            JLabel streetLabel = new JLabel(tr("Street"));
+            JLabel buildingLabel = new JLabel(tr("Building"));
+            JLabel loLabel = new JLabel(tr("Lowest Number"));
             loLabel.setPreferredSize(new Dimension(111, 16));
             loLabel.setToolTipText(tr("Lowest housenumber of the terraced house"));
-            hiLabel = new JLabel(tr("Highest Number"));
+            JLabel hiLabel = new JLabel(tr("Highest Number"));
             numbersLabel = new JLabel(tr("List of Numbers"));
             loLabel.setPreferredSize(new Dimension(111, 16));
             final String txt = relationExists ? tr("add to existing associatedStreet relation") : tr("create an associatedStreet relation");
 
-            handleRelationCheckBox = new JCheckBox(txt, relationExists ? Config.getPref().getBoolean(HANDLE_RELATION, true) : false);
+            handleRelationCheckBox = new JCheckBox(txt, relationExists && Config.getPref().getBoolean(HANDLE_RELATION, true));
             keepOutlineCheckBox = new JCheckBox(tr("keep outline way"), Config.getPref().getBoolean(KEEP_OUTLINE, false));
 
@@ -187 +185 @@
 
             inputPanel.add(loLabel, GBC.std().insets(3, 3, 0, 0));
-            inputPanel.add(getLo(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0));
+            inputPanel.add(getLo(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0));
             inputPanel.add(hiLabel, GBC.std().insets(3, 3, 0, 0));
-            inputPanel.add(getHi(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0));
+            inputPanel.add(getHi(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0));
             inputPanel.add(numbersLabel, GBC.std().insets(3, 3, 0, 0));
-            inputPanel.add(getNumbers(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0));
+            inputPanel.add(getNumbers(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0));
             inputPanel.add(interpolationLabel, GBC.std().insets(3, 3, 0, 0));
             inputPanel.add(getInterpolation(), GBC.eol().insets(5, 3, 0, 0));
             inputPanel.add(segmentsLabel, GBC.std().insets(3, 3, 0, 0));
-            inputPanel.add(getSegments(), GBC.eol().fill(GBC.HORIZONTAL).insets(5, 3, 0, 0));
+            inputPanel.add(getSegments(), GBC.eol().fill(GridBagConstraints.HORIZONTAL).insets(5, 3, 0, 0));
             if (streetName == null) {
                 inputPanel.add(streetLabel, GBC.std().insets(3, 3, 0, 0));
@@ -215 +213 @@
                 hi.setEnabled(false);
                 interpolationLabel.setVisible(false);
-                interpolation.setVisible(false);
-                interpolation.setEnabled(false);
-                segments.setText(String.valueOf(housenumbers.size()));
+                interpolationType.setVisible(false);
+                interpolationType.setEnabled(false);
+                segments.setText(String.valueOf(houseNumbers.size()));
                 segments.setEditable(false);
             }
@@ -267 +265 @@
             numbers = new JTextField();
 
-            Iterator<Node> it = housenumbers.iterator();
+            Iterator<Node> it = houseNumbers.iterator();
             StringBuilder s = new StringBuilder(256);
             if (it.hasNext()) {
@@ -338 +336 @@
      * @return java.awt.Choice
      */
-    private Choice getInterpolation() {
-        if (interpolation == null) {
-            interpolation = new Choice();
-            interpolation.add(tr("All"));
-            interpolation.add(tr("Even/Odd"));
+    private JComponent getInterpolation() {
+        if (interpolationType == null) {
+            interpolationType = new JosmComboBox<>();
+            interpolationType.setEditable(false);
+            interpolationType.addItem(tr("All"));
+            interpolationType.addItem(tr("Even/Odd"));
             if (Config.getPref().getInt(INTERPOLATION, 2) == 1) {
-                interpolation.select(tr("All"));
+                interpolationType.setSelectedItemText(tr("All"));
             } else {
-                interpolation.select(tr("Even/Odd"));
-            }
-        }
-        return interpolation;
+                interpolationType.setSelectedItemText(tr("Even/Odd"));
+            }
+        }
+        return interpolationType;
     }
 
@@ -355 +354 @@
      * Generates a list of all visible names of highways in order to do
      * autocompletion on the road name.
-     */
-    TreeSet<String> createAutoCompletionInfo() {
+     * @return The visible names
+     */
+    Set<String> createAutoCompletionInfo() {
         final TreeSet<String> names = new TreeSet<>();
         for (OsmPrimitive osm : MainApplication.getLayerManager().getEditDataSet()

applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/HouseNumberInputHandler.java

@@ -8 +8 @@
 import java.awt.Container;
 import java.awt.event.ActionEvent;
-import java.awt.event.ActionListener;
 import java.awt.event.FocusEvent;
 import java.awt.event.FocusListener;
 import java.awt.event.ItemEvent;
 import java.awt.event.ItemListener;
-import java.util.ArrayList;
+import java.util.List;
 
 import javax.swing.JButton;
-import javax.swing.JOptionPane;
 import javax.swing.JTextField;
 
@@ -26 +24 @@
 import org.openstreetmap.josm.gui.tagging.ac.AutoCompComboBox;
 import org.openstreetmap.josm.spi.preferences.Config;
-import org.openstreetmap.josm.tools.Logging;
-import org.openstreetmap.josm.tools.UserCancelException;
 import org.openstreetmap.josm.tools.Utils;
 
@@ -33 +29 @@
  * The Class HouseNumberInputHandler contains all the logic
  * behind the house number input dialog.
- *
+ * <p>
  * From a refactoring viewpoint, this class is indeed more interested in the fields
  * of the HouseNumberInputDialog. This is desired design, as the HouseNumberInputDialog
@@ -40 +36 @@
  * @author casualwalker - Copyright 2009 CloudMade Ltd
  */
-public class HouseNumberInputHandler extends JosmAction implements ActionListener, FocusListener, ItemListener {
+public class HouseNumberInputHandler extends JosmAction implements FocusListener, ItemListener {
     private final TerracerAction terracerAction;
-    private final Way outline, street;
+    private final Way outline;
+    private final Way street;
     private final String streetName;
     private final Node init;
     private final Relation associatedStreet;
-    private final ArrayList<Node> housenumbers;
-    public HouseNumberInputDialog dialog;
+    private final List<Node> housenumbers;
+    HouseNumberInputDialog dialog;
 
     /**
@@ -67 +64 @@
             final Way outline, final Node init, final Way street, final String streetName, final String buildingType,
             final Relation associatedStreet,
-            final ArrayList<Node> housenumbers, final String title) {
+            final List<Node> housenumbers, final String title) {
         this.terracerAction = terracerAction;
         this.outline = outline;
@@ -109 +106 @@
      * When the validation fails, a red message is
      * displayed and the OK button is disabled.
-     *
+     * <p>
      * Should be triggered each time the input changes.
+     * @return {@code true} if the inputs are ok
      */
     private boolean validateInput() {
         boolean isOk = true;
-        StringBuffer message = new StringBuffer();
-
-        isOk = isOk && checkNumberOrder(message);
-        isOk = isOk && checkSegmentsFromHousenumber(message);
-        isOk = isOk && checkSegments(message);
+        final StringBuilder message = new StringBuilder();
+
+        isOk &= checkNumberOrder(message);
+        isOk &= checkSegmentsFromHousenumber(message);
+        isOk &= checkSegments(message);
 
         // Allow non numeric characters for the low number as long as there is
         // no high number of the segmentcount is 1
         if (dialog.hi.getText().length() > 0 && (segments() != null || segments() < 1)) {
-            isOk = isOk
-                    && checkNumberStringField(dialog.lo, tr("Lowest number"),
+            isOk &= checkNumberStringField(dialog.lo, tr("Lowest number"),
                             message);
         }
-        isOk = isOk
-                && checkNumberStringField(dialog.hi, tr("Highest number"),
+        isOk &= checkNumberStringField(dialog.hi, tr("Highest number"),
                         message);
-        isOk = isOk
-                && checkNumberStringField(dialog.segments, tr("Segments"),
+        isOk &= checkNumberStringField(dialog.segments, tr("Segments"),
                         message);
 
+        JButton okButton = getButton(dialog, "OK");
+        if (okButton != null)
+            okButton.setEnabled(isOk);
         if (isOk) {
-            JButton okButton = getButton(dialog, "OK");
-            if (okButton != null)
-                okButton.setEnabled(true);
 
             // For some reason the messageLabel doesn't want to show up
@@ -144 +139 @@
             return true;
         } else {
-            JButton okButton = getButton(dialog, "OK");
-            if (okButton != null)
-                okButton.setEnabled(false);
-
             // For some reason the messageLabel doesn't want to show up, so a
             // MessageDialog is shown instead. Someone more knowledgeable might fix this.
@@ -168 +159 @@
      * @return true, if successful
      */
-    private boolean checkNumberOrder(final StringBuffer message) {
-        if (numberFrom() != null && numberTo() != null) {
-            if (numberFrom().intValue() > numberTo().intValue()) {
-                appendMessageNewLine(message);
-                message.append(tr("Lowest housenumber cannot be higher than highest housenumber"));
-                return false;
-            }
+    private boolean checkNumberOrder(final StringBuilder message) {
+        if (numberFrom() != null && numberTo() != null && numberFrom() > numberTo()) {
+            appendMessageNewLine(message);
+            message.append(tr("Lowest housenumber cannot be higher than highest housenumber"));
+            return false;
         }
         return true;
@@ -182 +171 @@
      * Obtain the number segments from the house number fields and check,
      * if they are valid.
-     *
+     * <p>
      * Also disables the segments field, if the house numbers contain
      * valid information.
@@ -190 +179 @@
      * @return true, if successful
      */
-    private boolean checkSegmentsFromHousenumber(final StringBuffer message) {
+    private boolean checkSegmentsFromHousenumber(final StringBuilder message) {
         if (!dialog.numbers.isVisible()) {
             dialog.segments.setEditable(true);
 
             if (numberFrom() != null && numberTo() != null) {
-                int segments = numberTo().intValue() - numberFrom().intValue();
+                int segments = numberTo() - numberFrom();
 
                 if (segments % stepSize() != 0) {
@@ -222 +211 @@
      * @return true, if successful
      */
-    private boolean checkSegments(final StringBuffer message) {
-        if (segments() == null || segments().intValue() < 1) {
+    private boolean checkSegments(final StringBuilder message) {
+        if (segments() == null || segments() < 1) {
             appendMessageNewLine(message);
             message.append(tr("Segment must be a number greater 1"));
@@ -241 +230 @@
      * @return true, if successful
      */
-    private boolean checkNumberStringField(final JTextField field,
-            final String label, final StringBuffer message) {
+    private static boolean checkNumberStringField(final JTextField field,
+            final String label, final StringBuilder message) {
         final String content = field.getText();
         if (content != null && content.length() != 0) {
@@ -267 +256 @@
      * @param message the message
      */
-    private void appendMessageNewLine(final StringBuffer message) {
+    private static void appendMessageNewLine(final StringBuilder message) {
         if (message.length() > 0) {
             message.append("\n");
@@ -287 +276 @@
                     saveValues();
 
-                    try {
-                        terracerAction.terraceBuilding(
-                            outline,
-                            init,
-                            street,
-                            associatedStreet,
-                            segments(),
-                            dialog.lo.getText(),
-                            dialog.hi.getText(),
-                            stepSize(),
-                            housenumbers,
-                            streetName(),
-                            doHandleRelation(),
-                            doKeepOutline(), buildingType());
-                    } catch (UserCancelException ex) {
-                        Logging.trace(ex);
-                    }
+                    terracerAction.terraceBuilding(
+                        outline,
+                        init,
+                        street,
+                        associatedStreet,
+                        segments(),
+                        dialog.lo.getText(),
+                        dialog.hi.getText(),
+                        stepSize(),
+                        housenumbers,
+                        streetName(),
+                        doHandleRelation(),
+                        doKeepOutline(), buildingType());
 
                     this.dialog.setVisible(false);
@@ -323 +308 @@
      */
     public Integer stepSize() {
-        return dialog.interpolation.getSelectedItem().equals(tr("All")) ? 1 : 2;
+        return tr("All").equals(dialog.interpolationType.getSelectedItem()) ? 1 : 2;
     }
 
@@ -405 +390 @@
      * Whether the user likes to create a relation or add to
      * an existing one.
+     * @return {@code true} if the user wants to create a relation
      */
     public boolean doHandleRelation() {
-        if (this.dialog == null) {
-            JOptionPane.showMessageDialog(null, "dialog", "alert", JOptionPane.ERROR_MESSAGE);
-        }
-        if (this.dialog.handleRelationCheckBox == null) {
-            JOptionPane.showMessageDialog(null, "checkbox", "alert", JOptionPane.ERROR_MESSAGE);
-            return true;
-        } else {
-            return this.dialog.handleRelationCheckBox.isSelected();
-        }
+        return this.dialog.handleRelationCheckBox.isSelected();
     }
 
     /**
      * Whether the user likes to keep the outline way.
+     * @return {@code true} if the user wants to keep the selected outline
      */
     public boolean doKeepOutline() {

applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/ReverseTerraceAction.java

@@ -6 +6 @@
 import java.awt.event.ActionEvent;
 import java.awt.event.KeyEvent;
+import java.util.ArrayDeque;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Deque;
 import java.util.HashSet;
 import java.util.LinkedList;
+import java.util.Set;
 
 import javax.swing.JOptionPane;
@@ -26 +29 @@
 /**
  * Tool to reverse the house numbers in a terrace.
- *
+ * <p>
  * Useful for when you're using the Terracer tool and the house numbers come out
  * in the wrong direction, or when someone has added house numbers in the wrong
  * direction anyway.
- *
- * Finds all connected ways which have a building=* tag on them in order (breadth
+ * <p>
+ * Finds all connected ways which have a building=* and addr tag on them in order (breadth
  * first search) and then changes the tags to be the reverse of the order in which
  * they were found.
  */
 public class ReverseTerraceAction extends JosmAction {
+    private static final String ADDR_HOUSENUMBER = "addr:housenumber";
 
+    /**
+     * Create a new action for reversing a terrace
+     */
     public ReverseTerraceAction() {
         super(tr("Reverse a terrace"),
@@ -54 +61 @@
     public void actionPerformed(ActionEvent e) {
         Collection<Way> selectedWays = MainApplication.getLayerManager().getEditDataSet().getSelectedWays();
+        reverseTerracedAddresses(selectedWays);
+    }
 
+    static void reverseTerracedAddresses(Collection<Way> selectedWays) {
         // Set to keep track of all the nodes that have been visited - that is: if
         // we encounter them again we will not follow onto the connected ways.
-        HashSet<Node> visitedNodes = new HashSet<>();
+        Set<Node> visitedNodes = new HashSet<>();
 
         // Set to keep track of the ways the algorithm has seen, but not yet visited.
         // Since when a way is visited all of its nodes are marked as visited, there
         // is no need to keep a visitedWays set.
-        HashSet<Way> front = new HashSet<>();
-
-        // Find the first or last way from the teracced houses.
-        // It should be connected to exactly one other way.
-        for (Way w : selectedWays) {
-            int conn = 0;
-            for (Way v : selectedWays) {
-                if (w.equals(v)) continue;
-                if (!Collections.disjoint(w.getNodes(), v.getNodes())) {
-                    ++conn;
-                }
-            }
-            if (conn == 1) {
-                front.add(w);
-                break;
-            }
-        }
+        final Deque<Way> front = findFirstWay(selectedWays);
 
         if (front.isEmpty()) {
@@ -86 +80 @@
         }
 
+
         // This is like a visitedWays set, but in a linear order.
         LinkedList<Way> orderedWays = new LinkedList<>();
@@ -92 +87 @@
         LinkedList<String> houseNumbers = new LinkedList<>();
 
-        while (front.size() > 0) {
+        while (!front.isEmpty()) {
             // Java apparently doesn't have useful methods to get single items from sets...
-            Way w = front.iterator().next();
+            Way w = front.pop();
 
             // Visit all the nodes in the way, adding the building's they're members of
@@ -101 +96 @@
                 if (!visitedNodes.contains(n)) {
                     for (OsmPrimitive prim : n.getReferrers()) {
-                        if (prim.keySet().contains("building") && prim instanceof Way) {
+                        if (prim.hasKey("building") && prim.hasKey(ADDR_HOUSENUMBER)
+                                && prim instanceof Way && !front.contains(prim)) {
                             front.add((Way) prim);
                         }
@@ -111 +107 @@
             // We've finished visiting this way, so record the attributes we're interested
             // in for re-writing.
-            front.remove(w);
             orderedWays.addLast(w);
-            houseNumbers.addFirst(w.get("addr:housenumber"));
+            houseNumbers.addFirst(w.get(ADDR_HOUSENUMBER));
         }
 
@@ -120 +115 @@
             commands.add(new ChangePropertyCommand(
                     orderedWays.get(i),
-                    "addr:housenumber",
+                    ADDR_HOUSENUMBER,
                     houseNumbers.get(i)));
         }
@@ -128 +123 @@
     }
 
+    private static Deque<Way> findFirstWay(Collection<Way> selectedWays) {
+        // Find the first or last way from the terraced houses.
+        // It should be connected to exactly one other way.
+        for (Way w : selectedWays) {
+            int conn = 0;
+            for (Way v : selectedWays) {
+                if (!w.equals(v) && !Collections.disjoint(w.getNodes(), v.getNodes())) {
+                    ++conn;
+                    if (conn > 1) {
+                        break;
+                    }
+                }
+            }
+            if (conn == 1) {
+                return new ArrayDeque<>(Collections.singletonList(w));
+            }
+        }
+        return new ArrayDeque<>();
+    }
+
     @Override
     protected void updateEnabledState() {

applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerAction.java

@@ -14 +14 @@
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
@@ -55 +54 @@
  * a street (highway=*, name=*) then the given street will be added
  * to the 'associatedStreet' relation.
- *
+ * <p>
  *
  * At present it only works on quadrilaterals, but there is no reason
@@ -64 +63 @@
  */
 public final class TerracerAction extends JosmAction {
+    private static final String BUILDING = "building";
+    private static final String ADDR_HOUSENUMBER = "addr:housenumber";
+    private static final String ADDR_STREET = "addr:street";
 
     private Collection<Command> commands;
@@ -123 +125 @@
             } else if (sel.size() > 1) {
                 List<Way> ways = new ArrayList<>(Utils.filteredCollection(sel, Way.class));
-                Iterator<Way> wit = ways.iterator();
-                while (wit.hasNext()) {
-                    Way way = wit.next();
-                    if (way.hasKey("building")) {
+                for (Way way : ways) {
+                    if (way.hasKey(BUILDING)) {
                         if (outline != null)
                             // already have a building
@@ -140 +140 @@
                             throw new InvalidUserInputException("street does not have any name");
                     } else
-                        throw new InvalidUserInputException(way+" is neither a building nor a highway");
+                        throw new InvalidUserInputException(way + " is neither a building nor a highway");
                 }
 
@@ -147 +147 @@
 
                 List<Node> nodes = new ArrayList<>(Utils.filteredCollection(sel, Node.class));
-                Iterator<Node> nit = nodes.iterator();
                 // Actually this should test if the selected address nodes lie
                 // within the selected outline. Any ideas how to do this?
-                while (nit.hasNext()) {
-                    Node node = nit.next();
-                    if (node.hasKey("addr:housenumber")) {
-                        String nodesStreetName = node.get("addr:street");
+                for (Node node : nodes) {
+                    if (node.hasKey(ADDR_HOUSENUMBER)) {
+                        String nodesStreetName = node.get(ADDR_STREET);
                         // if a node has a street name if must be equal
                         // to the one of the other address nodes
@@ -174 +172 @@
                 }
 
-                Collections.sort(housenumbers, new HousenumberNodeComparator());
+                housenumbers.sort(new HousenumberNodeComparator());
             }
 
@@ -182 +180 @@
         } catch (InvalidUserInputException ex) {
             Logging.warn("Terracer: "+ex.getMessage());
-            new ExtendedDialog(MainApplication.getMainFrame(), tr("Invalid selection"), new String[] {"OK"})
-                .setButtonIcons(new String[] {"ok"}).setIcon(JOptionPane.INFORMATION_MESSAGE)
+            new ExtendedDialog(MainApplication.getMainFrame(), tr("Invalid selection"), "OK")
+                .setButtonIcons("ok").setIcon(JOptionPane.INFORMATION_MESSAGE)
                 .setContent(tr("Select a single, closed way of at least four nodes. " +
                     "(Optionally you can also select a street for the addr:street tag " +
@@ -217 +215 @@
             // Special case of one outline and one address node.
             // Don't open the dialog
-            try {
-                terraceBuilding(outline, init, street, associatedStreet, 0, null, null, 0,
-                        housenumbers, streetname, associatedStreet != null, false, "yes");
-            } catch (UserCancelException ex) {
-                Logging.trace(ex);
-            }
+            terraceBuilding(outline, init, street, associatedStreet, 0, null, null, 0,
+                    housenumbers, streetname, associatedStreet != null, false, "yes");
         } else {
             String title = trn("Change {0} object", "Change {0} objects", sel.size(), sel.size());
             // show input dialog.
-            new HouseNumberInputHandler(this, outline, init, street, streetname, outline.get("building"),
+            new HouseNumberInputHandler(this, outline, init, street, streetname, outline.get(BUILDING),
                     associatedStreet, housenumbers, title).dialog.showDialog();
         }
@@ -238 +232 @@
     }
 
-    public Integer getNumber(String number) {
+    private static Integer getNumber(String number) {
         try {
             return Integer.parseInt(number);
@@ -251 +245 @@
      */
     static class HousenumberNodeComparator implements Comparator<Node> {
-        private final Pattern pat = Pattern.compile("^(\\d+)\\s*(.*)");
+        private static final Pattern PATTERN_HOUSE_NUMBER = Pattern.compile("^(\\d+)\\s*(.*)", Pattern.UNICODE_CHARACTER_CLASS);
 
         @Override
@@ -259 +253 @@
             // doesn't work for numbers with different number of digits,
             // e.g. 9 is higher than 11
-            String node1String = node1.get("addr:housenumber");
-            String node2String = node2.get("addr:housenumber");
-            Matcher mat = pat.matcher(node1String);
+            String node1String = node1.get(ADDR_HOUSENUMBER);
+            String node2String = node2.get(ADDR_HOUSENUMBER);
+            Matcher mat = PATTERN_HOUSE_NUMBER.matcher(node1String);
             if (mat.find()) {
                 Integer node1Int = Integer.valueOf(mat.group(1));
                 String node1Rest = mat.group(2);
-                mat = pat.matcher(node2String);
+                mat = PATTERN_HOUSE_NUMBER.matcher(node2String);
                 if (mat.find()) {
                     Integer node2Int = Integer.valueOf(mat.group(1));
@@ -285 +279 @@
     /**
      * Terraces a single, closed, quadrilateral way.
-     *
+     * <p>
      * Any node must be adjacent to both a short and long edge, we naively
      * choose the longest edge and its opposite and interpolate along them
@@ -307 +301 @@
      * @param keepOutline If the outline way should be kept
      * @param buildingValue The value for {@code building} key to add
-     * @throws UserCancelException if user cancels the operation
      */
     public void terraceBuilding(final Way outline, Node init, Way street, Relation associatedStreet, Integer segments,
                 String start, String end, int step, List<Node> housenumbers, String streetName, boolean handleRelations,
-                boolean keepOutline, String buildingValue) throws UserCancelException {
+                boolean keepOutline, String buildingValue) {
         final int nb;
-        Integer to = null, from = null;
+        Integer to;
+        Integer from = null;
         if (housenumbers == null || housenumbers.isEmpty()) {
             to = getNumber(end);
             from = getNumber(start);
             if (to != null && from != null) {
-                nb = 1 + (to.intValue() - from.intValue()) / step;
+                nb = 1 + (to - from) / step;
             } else if (segments != null) {
-                nb = segments.intValue();
+                nb = segments;
             } else {
                 // if we get here, there is is a bug in the input validation.
@@ -335 +329 @@
         Pair<Way, Way> interp = findFrontAndBack(outline);
 
-        final boolean swap = init != null && (interp.a.lastNode().equals(init) || interp.b.lastNode().equals(init));
+        final boolean swap = init != null && (init.equals(interp.a.lastNode()) || init.equals(interp.b.lastNode()));
 
         final double frontLength = wayLength(interp.a);
@@ -418 +412 @@
         // Remove the address nodes since their tags have been incorporated into the terraces.
         // Or should removing them also be an option?
-        if (!housenumbers.isEmpty()) {
+        if (housenumbers != null && !housenumbers.isEmpty()) {
             commands.add(DeleteCommand.delete(housenumbers, true, true));
         }
@@ -511 +505 @@
      * @param associatedStreet The associated street. Used to determine if addr:street should be set or not.
      * @param buildingValue The value for {@code building} key to add
-     * @throws UserCancelException if user cancels the operation
+     * @param houseNumbers The house numbers to use
+     * @param i The index to use in {@code houseNumbers} for a replacement house number (preferential)
+     * @param defaultNumber The number to use if there was not an underlying house number
      */
     private void addressBuilding(Way outline, Way street, String streetName, Relation associatedStreet,
-            List<Node> housenumbers, int i, String defaultNumber, String buildingValue) throws UserCancelException {
-        Node houseNum = (housenumbers != null && i >= 0 && i < housenumbers.size()) ? housenumbers.get(i) : null;
+            List<Node> houseNumbers, int i, String defaultNumber, String buildingValue) {
+        Node houseNum = (houseNumbers != null && i >= 0 && i < houseNumbers.size()) ? houseNumbers.get(i) : null;
         boolean buildingAdded = false;
         boolean numberAdded = false;
         Map<String, String> tags = new HashMap<>();
         if (houseNum != null) {
-            primitives = Arrays.asList(new OsmPrimitive[]{houseNum, outline});
+            primitives = Arrays.asList(houseNum, outline);
 
             TagCollection tagsToCopy = TagCollection.unionOfAllPrimitives(primitives).getTagsFor(houseNum.keySet());
@@ -530 +526 @@
             }
 
-            buildingAdded = houseNum.hasKey("building");
-            numberAdded = houseNum.hasKey("addr:housenumber");
+            buildingAdded = houseNum.hasKey(BUILDING);
+            numberAdded = houseNum.hasKey(ADDR_HOUSENUMBER);
         }
         if (!buildingAdded && buildingValue != null && !buildingValue.isEmpty()) {
-            tags.put("building", buildingValue);
+            tags.put(BUILDING, buildingValue);
         }
         if (defaultNumber != null && !numberAdded) {
-            tags.put("addr:housenumber", defaultNumber);
+            tags.put(ADDR_HOUSENUMBER, defaultNumber);
         }
         // Only put addr:street if no relation exists or if it has no name
         if (associatedStreet == null || !associatedStreet.hasKey("name")) {
             if (street != null) {
-                tags.put("addr:street", street.get("name"));
+                tags.put(ADDR_STREET, street.get("name"));
             } else if (streetName != null && !streetName.trim().isEmpty()) {
-                tags.put("addr:street", streetName.trim());
+                tags.put(ADDR_STREET, streetName.trim());
             }
         }
@@ -555 +551 @@
      * Creates a node at a certain distance along a way, as calculated by the
      * great circle distance.
-     *
+     * <p>
      * Note that this really isn't an efficient way to do this and leads to
      * O(N^2) running time for the main algorithm, but its simple and easy
@@ -564 +560 @@
      * @return A node at a distance l along w from the first point.
      */
-    private Node interpolateAlong(Way w, double l) {
+    private static Node interpolateAlong(Way w, double l) {
         List<Pair<Node, Node>> pairs = w.getNodePairs(false);
         for (int i = 0; i < pairs.size(); ++i) {
@@ -587 +583 @@
      * @return The length of the way.
      */
-    private double wayLength(Way w) {
+    private static double wayLength(Way w) {
         double length = 0.0;
         for (Pair<Node, Node> p : w.getNodePairs(false)) {
@@ -603 +599 @@
      * @return A pair of ways (front, back) pointing in the same directions.
      */
-    private Pair<Way, Way> findFrontAndBack(Way w) {
+    private static Pair<Way, Way> findFrontAndBack(Way w) {
         // calculate the "side-ness" score for each segment of the way
         double[] sideness = calculateSideness(w);
@@ -658 +654 @@
     /**
      * returns the distance of two segments of a closed polygon
-     */
-    private int indexDistance(int i1, int i2, int n) {
+     * @param i1 The first segment index
+     * @param i2 The second segment index
+     * @param n The number of segments in the polygon
+     * @return The distance between the two segments
+     */
+    private static int indexDistance(int i1, int i2, int n) {
         return Math.min(positiveModulus(i1 - i2, n), positiveModulus(i2 - i1, n));
     }
@@ -665 +665 @@
     /**
      * return the modulus in the range [0, n)
-     */
-    private int positiveModulus(int a, int n) {
+     * @param a dividend
+     * @param n divisor
+     * @return The positive modulus (if {@code a} is negative)
+     */
+    private static int positiveModulus(int a, int n) {
         if (n <= 0)
             throw new IllegalArgumentException();
@@ -679 +682 @@
      * Calculate the length of a side (from node i to i+1) in a way. This assumes that
      * the way is closed, but I only ever call it for buildings.
-     */
-    private double sideLength(Way w, int i) {
+     * @param w The way
+     * @param i The side (0 indexed)
+     * @return The length of that way segment
+     */
+    private static double sideLength(Way w, int i) {
         Node a = w.getNode(i);
         Node b = w.getNode((i + 1) % (w.getNodesCount() - 1));
@@ -690 +696 @@
      * into order and return the array of indexes such that, for a returned array
      * x, a[x[i]] is sorted for ascending index i.
-     *
+     * <p>
      * This isn't efficient at all, but should be fine for the small arrays we're
      * expecting. If this gets slow - replace it with some more efficient algorithm.
@@ -698 +704 @@
      * is in sorted order.
      */
-    private int[] sortedIndexes(final double[] a) {
-        class SortWithIndex implements Comparable<SortWithIndex> {
-            public double x;
-            public int i;
-
-            SortWithIndex(double a, int b) {
-                x = a;
-                i = b;
-            }
-
-            @Override
-            public int compareTo(SortWithIndex o) {
-                return Double.compare(x, o.x);
-            }
-        }
+    private static int[] sortedIndexes(final double[] a) {
 
         final int length = a.length;
@@ -731 +723 @@
     /**
      * Calculate "sideness" metric for each segment in a way.
-     */
-    private double[] calculateSideness(Way w) {
+     * @param w The way to get the sideness metric for
+     * @return The sideness for each segment of the way
+     */
+    private static double[] calculateSideness(Way w) {
         final int length = w.getNodesCount() - 1;
         double[] sideness = new double[length];
@@ -752 +746 @@
      * segment and its previous and next segments in order. Sideness is calculated
      * for the segment b-c.
-     */
-    private double calculateSideness(Node a, Node b, Node c, Node d) {
+     * @param a The previous node
+     * @param b The first node of the current segment
+     * @param c The last node of the current segment
+     * @param d The next node
+     * @return the sideness
+     */
+    private static double calculateSideness(ILatLon a, ILatLon b, ILatLon c, ILatLon d) {
         final double ndx = b.lon() - a.lon();
         final double pdx = d.lon() - c.lon();
@@ -766 +765 @@
      * Creates a new node at the interpolated position between the argument
      * nodes. Interpolates linearly in projected coordinates.
-     *
+     * <p>
      * If new node coordinate matches a or b coordinates, a or b is returned.
      *
@@ -774 +773 @@
      * @return A new node at the interpolated position (or a or b in case if f ≈ 0 or f ≈ 1).
      */
-    private Node interpolateNode(Node a, Node b, double f) {
+    private static Node interpolateNode(Node a, Node b, double f) {
         Node n = new Node(a.getEastNorth().interpolate(b.getEastNorth(), f));
-                if (n.equalsEpsilon(a, ILatLon.MAX_SERVER_PRECISION))
+        if (n.equalsEpsilon(a, ILatLon.MAX_SERVER_PRECISION))
             return a;
         if (n.equalsEpsilon(b, ILatLon.MAX_SERVER_PRECISION))
@@ -787 +786 @@
         setEnabled(getLayerManager().getEditDataSet() != null);
     }
+
+    private static class SortWithIndex implements Comparable<SortWithIndex> {
+        /**
+         * The value to sort
+         */
+        public final double x;
+        /**
+         * The index in the original array
+         */
+        public final int i;
+
+        SortWithIndex(double a, int b) {
+            x = a;
+            i = b;
+        }
+
+        @Override
+        public int compareTo(SortWithIndex o) {
+            return Double.compare(x, o.x);
+        }
+    }
 }

applications/editors/josm/plugins/terracer/src/org/openstreetmap/josm/plugins/terracer/TerracerPlugin.java

@@ -26 +26 @@
  */
 public class TerracerPlugin extends Plugin implements Destroyable {
-        private List<JosmAction> actions = Arrays.asList(new TerracerAction(), new ReverseTerraceAction());
+    private final List<JosmAction> actions = Arrays.asList(new TerracerAction(), new ReverseTerraceAction());
 
     public TerracerPlugin(PluginInformation info) {
         super(info);
         for (JosmAction action : actions) {
-                MainMenu.add(MainApplication.getMenu().moreToolsMenu, action);
+            MainMenu.add(MainApplication.getMenu().moreToolsMenu, action);
         }
     }
 
-        @Override
-        public void destroy() {
+    @Override
+    public void destroy() {
         final JMenu moreToolsMenu = MainApplication.getMenu().moreToolsMenu;
-        final Map<Action, Component> actionsMap = Arrays.asList(moreToolsMenu.getMenuComponents()).stream()
+        final Map<Action, Component> actionsMap = Arrays.stream(moreToolsMenu.getMenuComponents())
                 .filter(JMenuItem.class::isInstance).map(JMenuItem.class::cast)
                 .collect(Collectors.toMap(JMenuItem::getAction, component -> component));
@@ -47 +47 @@
             }
         }
-                actions.forEach(JosmAction::destroy);
-        }
+        actions.forEach(JosmAction::destroy);
+    }
 }