Merge pull request #189 from refactorfirst/improve-class-relationship-removal-calculation

Improve class relationship removal calculation

Author: jimbethancourt

codebase-graph-builder/src/main/java/org/hjug/graphbuilder/CodebaseGraphDTO.java

@@ -1,6 +1,5 @@
 package org.hjug.graphbuilder;
 
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -26,7 +25,6 @@ public class CodebaseGraphDTO {
 
     private final List<ClassDisharmony> classDisharmonies;
     private final List<MethodDisharmony> methodDisharmonies;
-    private final Map<String, Long> disharmonyCountByClass;
 
     public CodebaseGraphDTO(
             Graph<String, DefaultWeightedEdge> classReferencesGraph,
@@ -41,15 +39,6 @@ public CodebaseGraphDTO(
         this.classToSourceFilePathMapping = classToSourceFilePathMapping;
         this.classDisharmonies = classDisharmonies;
         this.methodDisharmonies = methodDisharmonies;
-        this.disharmonyCountByClass = buildDisharmonyIndex(classDisharmonies, methodDisharmonies);
-    }
-
-    private static Map<String, Long> buildDisharmonyIndex(
-            List<ClassDisharmony> classDisharmonies, List<MethodDisharmony> methodDisharmonies) {
-        Map<String, Long> counts = new HashMap<>();
-        classDisharmonies.forEach(d -> counts.merge(d.getMetrics().getClassName(), 1L, Long::sum));
-        methodDisharmonies.forEach(m -> counts.merge(m.getClassName(), 1L, Long::sum));
-        return counts;
     }
 
     public List<ClassDisharmony> getClassDisharmoniesOfType(String disharmonyType) {
@@ -63,8 +52,4 @@ public List<MethodDisharmony> getMethodDisharmoniesOfType(String disharmonyType)
                 .filter(d -> disharmonyType.equals(d.getDisharmonyType()))
                 .collect(Collectors.toList());
     }
-
-    public long getClassDisharmonyCountForClass(String classFqn) {
-        return disharmonyCountByClass.getOrDefault(classFqn, 0L);
-    }
 }

cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java

@@ -27,6 +27,7 @@
 import org.hjug.metrics.DisharmonyRanker;
 import org.hjug.metrics.rules.CBORule;
 import org.jgrapht.Graph;
+import org.jgrapht.graph.AsSubgraph;
 import org.jgrapht.graph.DefaultWeightedEdge;
 
 @Slf4j
@@ -330,9 +331,15 @@ public List<RankedDisharmony> calculateRelationshipCostBenefitValues(
             Graph<String, DefaultWeightedEdge> classGraph,
             Map<DefaultWeightedEdge, Integer> edgeToRemoveCycleCounts,
             CodebaseGraphDTO dto,
-            Set<String> vertexesToRemove) {
+            Set<String> vertexesToRemove,
+            Map<String, AsSubgraph<String, DefaultWeightedEdge>> packageCycles,
+            List<RankedDisharmony> packageRelationshipDisharmonies) {
         List<RankedDisharmony> edgesThatNeedToBeRemoved = new ArrayList<>();
 
+        Set<DefaultWeightedEdge> packageEdgesToRemove = packageRelationshipDisharmonies.stream()
+                .map(RankedDisharmony::getEdge)
+                .collect(Collectors.toSet());
+
         for (DefaultWeightedEdge edge : classGraph.edgeSet()) {
             // shouldn't have to check for null edges & counts :-(
             if (null == edge || null == edgeToRemoveCycleCounts.get(edge)) continue;
@@ -350,8 +357,8 @@ public List<RankedDisharmony> calculateRelationshipCostBenefitValues(
                     (int) classGraph.getEdgeWeight(edge),
                     sourceNodeShouldBeRemoved,
                     targetNodeShouldBeRemoved,
-                    dto.getClassDisharmonyCountForClass(edgeSource),
-                    dto.getClassDisharmonyCountForClass(edgeTarget));
+                    getPackageCycleCount(edgeSource, edgeTarget, dto, packageCycles),
+                    packageRelationshipShouldBeRemoved(edgeSource, edgeTarget, dto, packageEdgesToRemove));
 
             edgesThatNeedToBeRemoved.add(edgeThatNeedsToBeRemoved);
         }
@@ -376,18 +383,69 @@ public List<RankedDisharmony> calculateRelationshipCostBenefitValues(
         return edgesThatNeedToBeRemoved;
     }
 
+    /**
+     * Counts how many package cycles contain the package-level relationship corresponding to the given class (or
+     * package) edge - i.e. cycles where the edge between the source's and target's packages is itself part of the
+     * cycle, not merely cycles that happen to contain one of the endpoints.
+     */
+    private static int getPackageCycleCount(
+            String edgeSource,
+            String edgeTarget,
+            CodebaseGraphDTO dto,
+            Map<String, AsSubgraph<String, DefaultWeightedEdge>> packageCycles) {
+        String sourcePackage = toPackageName(edgeSource, dto);
+        String targetPackage = toPackageName(edgeTarget, dto);
+
+        int packageCycleCount = 0;
+        for (AsSubgraph<String, DefaultWeightedEdge> packageCycle : packageCycles.values()) {
+            if (packageCycle.containsEdge(sourcePackage, targetPackage)) {
+                packageCycleCount++;
+            }
+        }
+        return packageCycleCount;
+    }
+
+    /**
+     * Determines whether the package-level relationship corresponding to the given class (or package) edge is
+     * itself one of the package edges selected for removal, based on membership in the already-computed package
+     * relationship disharmonies.
+     */
+    private static boolean packageRelationshipShouldBeRemoved(
+            String edgeSource, String edgeTarget, CodebaseGraphDTO dto, Set<DefaultWeightedEdge> packageEdgesToRemove) {
+        String sourcePackage = toPackageName(edgeSource, dto);
+        String targetPackage = toPackageName(edgeTarget, dto);
+        DefaultWeightedEdge packageEdge = dto.getPackageReferencesGraph().getEdge(sourcePackage, targetPackage);
+        return packageEdge != null && packageEdgesToRemove.contains(packageEdge);
+    }
+
+    /**
+     * The vertex may already be a package name (when classGraph is actually a package graph) or a fully-qualified
+     * class name, in which case the containing package is derived from it.
+     */
+    private static String toPackageName(String vertex, CodebaseGraphDTO dto) {
+        if (dto.getPackageReferencesGraph().containsVertex(vertex)) {
+            return vertex;
+        } else if (vertex.contains(".")) {
+            return vertex.substring(0, vertex.lastIndexOf('.'));
+        } else {
+            return "";
+        }
+    }
+
     static void sortEdgesThatNeedToBeRemoved(List<RankedDisharmony> rankedDisharmonies) {
         // Sort by impact value
         // Order by cycle count reversed (highest count bubbles to the top)
         rankedDisharmonies.sort(Comparator.comparingInt(RankedDisharmony::getCycleCount)
                 .reversed()
                 // then by weight, with lowest weight edges bubbling to the top
                 .thenComparingInt(RankedDisharmony::getEffortRank)
-                // then by disharmony count
-                .thenComparingInt(RankedDisharmony::getEdgeSourceDisharmonyCount)
-                .thenComparingInt(RankedDisharmony::getEdgeTargetDisharmonyCount)
-                // then if the source node is in the list of nodes to be removed
+                // then by whether the underlying package relationship should also be removed, true before false
                 // multiplying by -1 reverses the sort order (reverse doesn't work in chained comparators)
+                .thenComparingInt(
+                        rankedDisharmony -> -1 * (rankedDisharmony.isPackageRelationshipShouldBeRemoved() ? 1 : 0))
+                // then by package cycle count, with classes in more package cycles bubbling to the top
+                .thenComparingInt(rankedDisharmony -> -1 * rankedDisharmony.getPackageCycleCount())
+                // then if the source node is in the list of nodes to be removed
                 .thenComparingInt(rankedDisharmony -> -1 * rankedDisharmony.getSourceNodeShouldBeRemoved())
                 // then if the target node is in the list of nodes to be removed
                 .thenComparingInt(rankedDisharmony -> -1 * rankedDisharmony.getTargetNodeShouldBeRemoved()));

cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java

@@ -44,8 +44,8 @@ public class RankedDisharmony {
     private int sourceNodeShouldBeRemoved;
     private int targetNodeShouldBeRemoved;
     private String edgeTargetClass;
-    private Integer edgeSourceDisharmonyCount;
-    private Integer edgeTargetDisharmonyCount;
+    private Integer packageCycleCount;
+    private boolean packageRelationshipShouldBeRemoved;
 
     public RankedDisharmony(GodClass godClass, ScmLogInfo scmLogInfo) {
         path = scmLogInfo.getPath();
@@ -109,14 +109,14 @@ public RankedDisharmony(
             int weight,
             boolean sourceNodeShouldBeRemoved,
             boolean targetNodeShouldBeRemoved,
-            long sourceDisharmonyCount,
-            long targetDisharmonyCount) {
+            int packageCycleCount,
+            boolean packageRelationshipShouldBeRemoved) {
 
         className = edgeSource;
         this.edge = edge;
         this.cycleCount = cycleCount;
-        edgeSourceDisharmonyCount = Math.toIntExact(sourceDisharmonyCount);
-        edgeTargetDisharmonyCount = Math.toIntExact(targetDisharmonyCount);
+        this.packageCycleCount = packageCycleCount;
+        this.packageRelationshipShouldBeRemoved = packageRelationshipShouldBeRemoved;
         effortRank = weight;
         this.sourceNodeShouldBeRemoved = sourceNodeShouldBeRemoved ? 1 : 0;
         this.targetNodeShouldBeRemoved = targetNodeShouldBeRemoved ? 1 : 0;