Refactor GroupAction to enforce Single Responsibility Principle

.github/workflows/ci.yml

@@ -0,0 +1,29 @@
+name: Java CI with Maven
+
+on:
+  pull_request:
+    branches: [ "develop" ]
+  push:
+    branches: [ "develop", "feature-branch" ]
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v3
+
+      - name: Set up JDK 11
+        uses: actions/setup-java@v3
+        with:
+          java-version: '11'
+          distribution: 'temurin'
+          cache: maven
+
+      - name: Build with Maven
+        run: mvn clean install -DskipTests --file pom.xml --settings .maven-settings.xml
+
+      - name: Run Tests
+        run: mvn test --file pom.xml --settings .maven-settings.xml
+

.maven-settings.xml

@@ -0,0 +1,9 @@
+<settings xmlns="http://maven.apache.org/SETTINGS/1.0.0">
+  <servers>
+    <server>
+      <id>github</id>
+      <username>${env.GITHUB_ACTOR}</username>
+      <password>${env.GITHUB_TOKEN}</password>
+    </server>
+  </servers>
+</settings>

jhotdraw-core/pom.xml

@@ -35,10 +35,43 @@
 			<version>6.8.21</version>
 			<scope>test</scope>
 		</dependency>
+		<dependency>
+			<groupId>junit</groupId>
+			<artifactId>junit</artifactId>
+			<version>4.13.2</version>
+			<scope>test</scope>
+		</dependency>
+		<dependency>
+			<groupId>org.mockito</groupId>
+			<artifactId>mockito-core</artifactId>
+			<version>4.11.0</version>
+			<scope>test</scope>
+		</dependency>
+		<dependency>
+			<groupId>com.tngtech.jgiven</groupId>
+			<artifactId>jgiven-junit</artifactId>
+			<version>0.18.2</version>
+			<scope>test</scope>
+		</dependency>
+		<dependency>
+			<groupId>org.assertj</groupId>
+			<artifactId>assertj-core</artifactId>
+			<version>3.24.2</version>
+			<scope>test</scope>
+		</dependency>
 	 <dependency>
 	  <groupId>${project.groupId}</groupId>
 	  <artifactId>jhotdraw-actions</artifactId>
 	  <version>${project.version}</version>
 	 </dependency>
 	</dependencies>
+	<build>
+		<plugins>
+			<plugin>
+				<groupId>org.apache.maven.plugins</groupId>
+				<artifactId>maven-surefire-plugin</artifactId>
+				<version>2.22.2</version>
+			</plugin>
+		</plugins>
+	</build>
 </project>

jhotdraw-core/src/main/java/org/jhotdraw/draw/action/GroupAction.java

@@ -7,160 +7,73 @@
  */
 package org.jhotdraw.draw.action;
 
-import org.jhotdraw.draw.figure.Figure;
+import java.awt.event.ActionEvent;
+import java.util.Collection;
+import java.util.LinkedList;
+import javax.swing.undo.AbstractUndoableEdit;
+import javax.swing.undo.CannotRedoException;
+import javax.swing.undo.CannotUndoException;
+import javax.swing.undo.UndoableEdit;
+import org.jhotdraw.draw.Drawing;
+import org.jhotdraw.draw.DrawingEditor;
+import org.jhotdraw.draw.DrawingView;
 import org.jhotdraw.draw.figure.CompositeFigure;
+import org.jhotdraw.draw.figure.Figure;
 import org.jhotdraw.draw.figure.GroupFigure;
-import java.util.*;
-import javax.swing.undo.*;
-import org.jhotdraw.draw.*;
 import org.jhotdraw.util.ResourceBundleUtil;
 
-/**
- * GroupAction.
- *
- * @author Werner Randelshofer
- * @version $Id$
- */
 public class GroupAction extends AbstractSelectedAction {
 
     private static final long serialVersionUID = 1L;
     public static final String ID = "edit.groupSelection";
     private CompositeFigure prototype;
-    /**
-     * If this variable is true, this action groups figures.
-     * If this variable is false, this action ungroups figures.
-     */
-    private boolean isGroupingAction;
-
-    /**
-     * Creates a new instance.
-     */
+
     public GroupAction(DrawingEditor editor) {
-        this(editor, new GroupFigure(), true);
+        this(editor, new GroupFigure());
     }
 
     public GroupAction(DrawingEditor editor, CompositeFigure prototype) {
-        this(editor, prototype, true);
-    }
-
-    public GroupAction(DrawingEditor editor, CompositeFigure prototype, boolean isGroupingAction) {
         super(editor);
         this.prototype = prototype;
-        this.isGroupingAction = isGroupingAction;
-        ResourceBundleUtil labels
-                = ResourceBundleUtil.getBundle("org.jhotdraw.draw.Labels");
+        ResourceBundleUtil labels = ResourceBundleUtil.getBundle("org.jhotdraw.draw.Labels");
         labels.configureAction(this, ID);
         updateEnabledState();
     }
 
     @Override
     protected void updateEnabledState() {
-        if (getView() != null) {
-            setEnabled(isGroupingAction ? canGroup() : canUngroup());
-        } else {
-            setEnabled(false);
-        }
+        setEnabled(canGroup(getView()));
     }
 
     protected boolean canGroup() {
-        return getView() != null && getView().getSelectionCount() > 1;
+        return canGroup(getView());
     }
 
-    protected boolean canUngroup() {
-        return getView() != null
-                && getView().getSelectionCount() == 1
-                && prototype != null
-                && getView().getSelectedFigures().iterator().next().getClass().equals(
-                        prototype.getClass());
+    protected boolean canGroup(DrawingView v) {
+        return v != null && v.getSelectionCount() > 1;
     }
 
     @Override
-    public void actionPerformed(java.awt.event.ActionEvent e) {
-        if (isGroupingAction) {
-            if (canGroup()) {
-                final DrawingView view = getView();
-                final LinkedList<Figure> ungroupedFigures = new LinkedList<>(view.getSelectedFigures());
-                final CompositeFigure group = (CompositeFigure) prototype.clone();
-                UndoableEdit edit = new AbstractUndoableEdit() {
-                    private static final long serialVersionUID = 1L;
-
-                    @Override
-                    public String getPresentationName() {
-                        ResourceBundleUtil labels
-                                = ResourceBundleUtil.getBundle("org.jhotdraw.draw.Labels");
-                        return labels.getString("edit.groupSelection.text");
-                    }
-
-                    @Override
-                    public void redo() throws CannotRedoException {
-                        super.redo();
-                        groupFigures(view, group, ungroupedFigures);
-                    }
-
-                    @Override
-                    public void undo() throws CannotUndoException {
-                        ungroupFigures(view, group);
-                        super.undo();
-                    }
-
-                    @Override
-                    public boolean addEdit(UndoableEdit anEdit) {
-                        return super.addEdit(anEdit);
-                    }
-                };
-                groupFigures(view, group, ungroupedFigures);
-                fireUndoableEditHappened(edit);
-            }
-        } else {
-            if (canUngroup()) {
-                final DrawingView view = getView();
-                final CompositeFigure group = (CompositeFigure) getView().getSelectedFigures().iterator().next();
-                final LinkedList<Figure> ungroupedFigures = new LinkedList<>();
-                UndoableEdit edit = new AbstractUndoableEdit() {
-                    private static final long serialVersionUID = 1L;
-
-                    @Override
-                    public String getPresentationName() {
-                        ResourceBundleUtil labels
-                                = ResourceBundleUtil.getBundle("org.jhotdraw.draw.Labels");
-                        return labels.getString("edit.ungroupSelection.text");
-                    }
-
-                    @Override
-                    public void redo() throws CannotRedoException {
-                        super.redo();
-                        ungroupFigures(view, group);
-                    }
-
-                    @Override
-                    public void undo() throws CannotUndoException {
-                        groupFigures(view, group, ungroupedFigures);
-                        super.undo();
-                    }
-                };
-                ungroupedFigures.addAll(ungroupFigures(view, group));
-                fireUndoableEditHappened(edit);
-            }
+    public void actionPerformed(ActionEvent e) {
+        if (canGroup()) {
+            final DrawingView view = getView();
+            final LinkedList<Figure> ungroupedFigures = new LinkedList<>(view.getSelectedFigures());
+            assert prototype.clone() instanceof CompositeFigure : "prototype.clone() must be a CompositeFigure";
+            final CompositeFigure group = (CompositeFigure) prototype.clone();
+
+            UndoableEdit edit = new GroupUndoableEdit(view, group, ungroupedFigures);
+            groupFigures(view, group, ungroupedFigures);
+            fireUndoableEditHappened(edit);
         }
     }
 
-    public Collection<Figure> ungroupFigures(DrawingView view, CompositeFigure group) {
-// XXX - This code is redundant with UngroupAction
-        LinkedList<Figure> figures = new LinkedList<>(group.getChildren());
-        view.clearSelection();
-        group.basicRemoveAllChildren();
-        view.getDrawing().basicAddAll(view.getDrawing().indexOf(group), figures);
-        view.getDrawing().remove(group);
-        view.addToSelection(figures);
-        return figures;
-    }
-
     public void groupFigures(DrawingView view, CompositeFigure group, Collection<Figure> figures) {
-        Collection<Figure> sorted = view.getDrawing().sort(figures);
-        int index = view.getDrawing().indexOf(sorted.iterator().next());
-        view.getDrawing().basicRemoveAll(figures);
+        Drawing drawing = view.getDrawing();
+        Collection<Figure> sorted = drawing.sort(figures);
+        int index = drawing.indexOf(sorted.iterator().next());
+        drawing.basicRemoveAll(figures);
         view.clearSelection();
-        view.getDrawing().add(index, group);
+        drawing.add(index, group);
         group.willChange();
         for (Figure f : sorted) {
             f.willChange();
@@ -169,4 +82,43 @@ public void groupFigures(DrawingView view, CompositeFigure group, Collection<Fig
         group.changed();
         view.addToSelection(group);
     }
+
+    // Extracted from anonymous inner class to eliminate Anonymous Inner Class smell
+    private class GroupUndoableEdit extends AbstractUndoableEdit {
+
+        private static final long serialVersionUID = 1L;
+        private final DrawingView view;
+        private final CompositeFigure group;
+        private final LinkedList<Figure> ungroupedFigures;
+
+        GroupUndoableEdit(DrawingView view, CompositeFigure group, LinkedList<Figure> ungroupedFigures) {
+            this.view = view;
+            this.group = group;
+            this.ungroupedFigures = ungroupedFigures;
+        }
+
+        @Override
+        public String getPresentationName() {
+            ResourceBundleUtil labels = ResourceBundleUtil.getBundle("org.jhotdraw.draw.Labels");
+            return labels.getString("edit.groupSelection.text");
+        }
+
+        @Override
+        public void redo() throws CannotRedoException {
+            super.redo();
+            groupFigures(view, group, ungroupedFigures);
+        }
+
+        @Override
+        public void undo() throws CannotUndoException {
+            Drawing drawing = view.getDrawing();
+            LinkedList<Figure> figures = new LinkedList<>(group.getChildren());
+            view.clearSelection();
+            group.basicRemoveAllChildren();
+            drawing.basicAddAll(drawing.indexOf(group), figures);
+            drawing.remove(group);
+            view.addToSelection(figures);
+            super.undo();
+        }
+    }
 }