From c64d25aed52776c2baee47ae4ca6c53f05e0a6c3 Mon Sep 17 00:00:00 2001 From: Jim Bethancourt Date: Tue, 30 Jun 2026 19:58:23 -0500 Subject: [PATCH 1/2] Capturing number of package cycles each class relationship to remove is involved in --- .../hjug/graphbuilder/CodebaseGraphDTO.java | 15 --- .../org/hjug/cbc/CostBenefitCalculator.java | 50 +++++++- .../java/org/hjug/cbc/RankedDisharmony.java | 9 +- .../hjug/cbc/CostBenefitCalculatorTest.java | 121 ++++++++++++++---- .../report/SimpleHtmlReport.java | 27 ++-- 5 files changed, 151 insertions(+), 71 deletions(-) diff --git a/codebase-graph-builder/src/main/java/org/hjug/graphbuilder/CodebaseGraphDTO.java b/codebase-graph-builder/src/main/java/org/hjug/graphbuilder/CodebaseGraphDTO.java index 8519332..9455ccb 100644 --- a/codebase-graph-builder/src/main/java/org/hjug/graphbuilder/CodebaseGraphDTO.java +++ b/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 classDisharmonies; private final List methodDisharmonies; - private final Map disharmonyCountByClass; public CodebaseGraphDTO( Graph classReferencesGraph, @@ -41,15 +39,6 @@ public CodebaseGraphDTO( this.classToSourceFilePathMapping = classToSourceFilePathMapping; this.classDisharmonies = classDisharmonies; this.methodDisharmonies = methodDisharmonies; - this.disharmonyCountByClass = buildDisharmonyIndex(classDisharmonies, methodDisharmonies); - } - - private static Map buildDisharmonyIndex( - List classDisharmonies, List methodDisharmonies) { - Map 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 getClassDisharmoniesOfType(String disharmonyType) { @@ -63,8 +52,4 @@ public List getMethodDisharmoniesOfType(String disharmonyType) .filter(d -> disharmonyType.equals(d.getDisharmonyType())) .collect(Collectors.toList()); } - - public long getClassDisharmonyCountForClass(String classFqn) { - return disharmonyCountByClass.getOrDefault(classFqn, 0L); - } } diff --git a/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java b/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java index 7e889f3..175824a 100644 --- a/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java +++ b/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,7 +331,8 @@ public List calculateRelationshipCostBenefitValues( Graph classGraph, Map edgeToRemoveCycleCounts, CodebaseGraphDTO dto, - Set vertexesToRemove) { + Set vertexesToRemove, + Map> packageCycles) { List edgesThatNeedToBeRemoved = new ArrayList<>(); for (DefaultWeightedEdge edge : classGraph.edgeSet()) { @@ -350,8 +352,7 @@ public List calculateRelationshipCostBenefitValues( (int) classGraph.getEdgeWeight(edge), sourceNodeShouldBeRemoved, targetNodeShouldBeRemoved, - dto.getClassDisharmonyCountForClass(edgeSource), - dto.getClassDisharmonyCountForClass(edgeTarget)); + getPackageCycleCount(edgeSource, edgeTarget, dto, packageCycles)); edgesThatNeedToBeRemoved.add(edgeThatNeedsToBeRemoved); } @@ -376,6 +377,42 @@ public List 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> packageCycles) { + String sourcePackage = toPackageName(edgeSource, dto); + String targetPackage = toPackageName(edgeTarget, dto); + + int packageCycleCount = 0; + for (AsSubgraph packageCycle : packageCycles.values()) { + if (packageCycle.containsEdge(sourcePackage, targetPackage)) { + packageCycleCount++; + } + } + return packageCycleCount; + } + + /** + * 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 rankedDisharmonies) { // Sort by impact value // Order by cycle count reversed (highest count bubbles to the top) @@ -383,11 +420,10 @@ static void sortEdgesThatNeedToBeRemoved(List rankedDisharmoni .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 package cycle count, with classes in more package cycles bubbling to the top // multiplying by -1 reverses the sort order (reverse doesn't work in chained comparators) + .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())); diff --git a/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java b/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java index aea907a..4a5a072 100644 --- a/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java +++ b/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java @@ -44,8 +44,7 @@ public class RankedDisharmony { private int sourceNodeShouldBeRemoved; private int targetNodeShouldBeRemoved; private String edgeTargetClass; - private Integer edgeSourceDisharmonyCount; - private Integer edgeTargetDisharmonyCount; + private Integer packageCycleCount; public RankedDisharmony(GodClass godClass, ScmLogInfo scmLogInfo) { path = scmLogInfo.getPath(); @@ -109,14 +108,12 @@ public RankedDisharmony( int weight, boolean sourceNodeShouldBeRemoved, boolean targetNodeShouldBeRemoved, - long sourceDisharmonyCount, - long targetDisharmonyCount) { + int packageCycleCount) { className = edgeSource; this.edge = edge; this.cycleCount = cycleCount; - edgeSourceDisharmonyCount = Math.toIntExact(sourceDisharmonyCount); - edgeTargetDisharmonyCount = Math.toIntExact(targetDisharmonyCount); + this.packageCycleCount = packageCycleCount; effortRank = weight; this.sourceNodeShouldBeRemoved = sourceNodeShouldBeRemoved ? 1 : 0; this.targetNodeShouldBeRemoved = targetNodeShouldBeRemoved ? 1 : 0; diff --git a/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java b/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java index a73dabd..10467bf 100644 --- a/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java +++ b/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java @@ -1,7 +1,6 @@ package org.hjug.cbc; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -15,6 +14,7 @@ import org.hjug.graphbuilder.CodebaseGraphDTO; import org.hjug.metrics.Disharmony; import org.jetbrains.annotations.NotNull; +import org.jgrapht.graph.AsSubgraph; import org.jgrapht.graph.DefaultWeightedEdge; import org.jgrapht.graph.SimpleDirectedWeightedGraph; import org.junit.jupiter.api.*; @@ -154,10 +154,11 @@ void calculateRelationshipCostBenefitValues_filtersMissingLogInfoAndAssignsPrior git.getRepository().getDirectory().getParent(), scmLogInfos)) { CodebaseGraphDTO dto = mock(CodebaseGraphDTO.class); - when(dto.getClassDisharmonyCountForClass(any())).thenReturn(0L); + when(dto.getPackageReferencesGraph()) + .thenReturn(new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class)); List disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove); + classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap()); Assertions.assertEquals(2, disharmonies.size()); @@ -176,7 +177,7 @@ void calculateRelationshipCostBenefitValues_filtersMissingLogInfoAndAssignsPrior Assertions.assertEquals(0, classC.getSourceNodeShouldBeRemoved()); Assertions.assertEquals(1, classC.getTargetNodeShouldBeRemoved()); Assertions.assertEquals(2, classC.getPriority().intValue()); - Assertions.assertEquals(0, classC.getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(0, classC.getPackageCycleCount()); } } @@ -226,16 +227,80 @@ void calculateRelationshipCostBenefitValues_prefersHigherChangePronenessRank() t git.getRepository().getDirectory().getParent(), scmLogInfos)) { CodebaseGraphDTO dto = mock(CodebaseGraphDTO.class); - when(dto.getClassDisharmonyCountForClass(any())).thenReturn(0L).thenReturn(1L); + when(dto.getPackageReferencesGraph()) + .thenReturn(new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class)); List disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove); + classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap()); Assertions.assertEquals(2, disharmonies.size()); - Assertions.assertEquals(0, disharmonies.get(0).getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(0, disharmonies.get(0).getPackageCycleCount()); Assertions.assertEquals(1, disharmonies.get(0).getPriority().intValue()); Assertions.assertEquals(2, disharmonies.get(1).getPriority().intValue()); - Assertions.assertEquals(1, disharmonies.get(1).getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(0, disharmonies.get(1).getPackageCycleCount()); + } + } + + @Test + void calculateRelationshipCostBenefitValues_packageCycleCountReflectsRelationshipNotJustSourcePackage() + throws Exception { + + writeFile(hudsonPath + "Placeholder.java", "public class Placeholder {}"); + git.add().addFilepattern(".").call(); + git.commit().setMessage("initial commit").call(); + + SimpleDirectedWeightedGraph packageGraph = + new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class); + packageGraph.addVertex("pkga"); + packageGraph.addVertex("pkgb"); + packageGraph.addVertex("pkgc"); + packageGraph.addVertex("pkgd"); + packageGraph.addEdge("pkga", "pkgb"); + packageGraph.addEdge("pkgb", "pkgc"); + packageGraph.addEdge("pkgc", "pkga"); + // dangling relationship: pkga depends on pkgd, but pkgd is not part of the pkga/pkgb/pkgc cycle + packageGraph.addEdge("pkga", "pkgd"); + + Map> packageCycles = new HashMap<>(); + packageCycles.put("cycle1", new AsSubgraph<>(packageGraph, Set.of("pkga", "pkgb", "pkgc"))); + + SimpleDirectedWeightedGraph classGraph = + new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class); + classGraph.addVertex("pkga.Foo"); + classGraph.addVertex("pkgb.Bar"); + classGraph.addVertex("pkgc.Baz"); + classGraph.addVertex("pkgd.Qux"); + + DefaultWeightedEdge fooToBar = classGraph.addEdge("pkga.Foo", "pkgb.Bar"); + DefaultWeightedEdge barToBaz = classGraph.addEdge("pkgb.Bar", "pkgc.Baz"); + DefaultWeightedEdge bazToFoo = classGraph.addEdge("pkgc.Baz", "pkga.Foo"); + DefaultWeightedEdge fooToQux = classGraph.addEdge("pkga.Foo", "pkgd.Qux"); + + Map edgeToRemoveCycleCounts = new HashMap<>(); + edgeToRemoveCycleCounts.put(fooToBar, 1); + edgeToRemoveCycleCounts.put(barToBaz, 1); + edgeToRemoveCycleCounts.put(bazToFoo, 1); + edgeToRemoveCycleCounts.put(fooToQux, 1); + + CodebaseGraphDTO dto = mock(CodebaseGraphDTO.class); + when(dto.getPackageReferencesGraph()).thenReturn(packageGraph); + + try (CostBenefitCalculator costBenefitCalculator = + new CostBenefitCalculator(git.getRepository().getDirectory().getParent(), new HashMap<>())) { + + List disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( + classGraph, edgeToRemoveCycleCounts, dto, Collections.emptySet(), packageCycles); + + Map disharmoniesByEdge = new HashMap<>(); + disharmonies.forEach(d -> disharmoniesByEdge.put(d.getEdge(), d)); + + Assertions.assertEquals(1, disharmoniesByEdge.get(fooToBar).getPackageCycleCount()); + Assertions.assertEquals(1, disharmoniesByEdge.get(barToBaz).getPackageCycleCount()); + Assertions.assertEquals(1, disharmoniesByEdge.get(bazToFoo).getPackageCycleCount()); + // pkga is in cycle1, but the Foo -> Qux relationship itself is not - must not be counted + Assertions.assertEquals(0, disharmoniesByEdge.get(fooToQux).getPackageCycleCount()); + } catch (Exception e) { + throw new RuntimeException(e); } } @@ -258,27 +323,27 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { logInfo5.setChangePronenessRank(5); // Create RankedDisharmony objects with different combinations - // Expected order after sorting: cycleCount desc, then sourceRemoved desc, then targetRemoved desc, then - // changeProneness desc - // cycle=5, source=0, target=0, change=5 + // Expected order after sorting: cycleCount desc, then effortRank asc, then packageCycleCount desc, + // then sourceRemoved desc, then targetRemoved desc + // cycle=5, source=0, target=0, packageCycleCount=6 RankedDisharmony disharmony1 = - new RankedDisharmony("Class1", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, false, false, 0, 0); + new RankedDisharmony("Class1", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, false, false, 6); - // cycle=5, source=1, target=0, change=3 + // cycle=5, source=1, target=0, packageCycleCount=1 RankedDisharmony disharmony2 = - new RankedDisharmony("Class2", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, true, false, 1, 1); + new RankedDisharmony("Class2", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, true, false, 1); - // cycle=3, source=0, target=1, change=8 + // cycle=3, source=0, target=1, packageCycleCount=5 RankedDisharmony disharmony3 = - new RankedDisharmony("Class3", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, true, 2, 2); + new RankedDisharmony("Class3", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, true, 5); - // cycle=3, source=0, target=0, change=2 + // cycle=3, source=0, target=0, packageCycleCount=0 RankedDisharmony disharmony4 = - new RankedDisharmony("Class4", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 6, 3); + new RankedDisharmony("Class4", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 0); - // cycle=3, source=0, target=0, change=5 + // cycle=3, source=0, target=0, packageCycleCount=2 RankedDisharmony disharmony5 = - new RankedDisharmony("Class5", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 5, 0); + new RankedDisharmony("Class5", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 2); List disharmonies = Arrays.asList(disharmony4, disharmony2, disharmony1, disharmony3, disharmony5); @@ -288,16 +353,16 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { // Verify the order // Order by cycle count reversed (highest count bubbles to the top) + // then Order by package cycle count (highest count bubbles to the top) // then Order by source node removed (source nodes needing to be removed bubble to the top) - // then Order by target node removed (target nodes needing to be removed bubble to the top)\ - // then Order by change proneness (highest change proneness bubbles to the top) + // then Order by target node removed (target nodes needing to be removed bubble to the top) for (RankedDisharmony disharmony : disharmonies) { System.out.println(disharmony.getClassName() + " " + disharmony.getCycleCount() + " " + disharmony.getEffortRank() + " " + disharmony.getSourceNodeShouldBeRemoved() + " " + disharmony.getTargetNodeShouldBeRemoved() + " " - + disharmony.getEdgeSourceDisharmonyCount()); + + disharmony.getPackageCycleCount()); } RankedDisharmony orderedDisharmony0 = disharmonies.get(0); @@ -306,7 +371,7 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { Assertions.assertEquals(1, orderedDisharmony0.getEffortRank().intValue()); Assertions.assertEquals(0, orderedDisharmony0.getSourceNodeShouldBeRemoved()); Assertions.assertEquals(0, orderedDisharmony0.getTargetNodeShouldBeRemoved()); - Assertions.assertEquals(0, orderedDisharmony0.getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(6, orderedDisharmony0.getPackageCycleCount()); RankedDisharmony orderedDisharmony1 = disharmonies.get(1); Assertions.assertEquals("Class2", orderedDisharmony1.getClassName()); @@ -314,7 +379,7 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { Assertions.assertEquals(1, orderedDisharmony1.getEffortRank().intValue()); Assertions.assertEquals(1, orderedDisharmony1.getSourceNodeShouldBeRemoved()); Assertions.assertEquals(0, orderedDisharmony1.getTargetNodeShouldBeRemoved()); - Assertions.assertEquals(1, orderedDisharmony1.getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(1, orderedDisharmony1.getPackageCycleCount()); RankedDisharmony orderedDisharmony2 = disharmonies.get(2); Assertions.assertEquals("Class3", orderedDisharmony2.getClassName()); @@ -322,7 +387,7 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { Assertions.assertEquals(1, orderedDisharmony2.getEffortRank().intValue()); Assertions.assertEquals(0, orderedDisharmony2.getSourceNodeShouldBeRemoved()); Assertions.assertEquals(1, orderedDisharmony2.getTargetNodeShouldBeRemoved()); - Assertions.assertEquals(2, orderedDisharmony2.getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(5, orderedDisharmony2.getPackageCycleCount()); RankedDisharmony orderedDisharmony3 = disharmonies.get(3); Assertions.assertEquals("Class5", orderedDisharmony3.getClassName()); @@ -330,7 +395,7 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { Assertions.assertEquals(1, orderedDisharmony3.getEffortRank().intValue()); Assertions.assertEquals(0, orderedDisharmony3.getSourceNodeShouldBeRemoved()); Assertions.assertEquals(0, orderedDisharmony3.getTargetNodeShouldBeRemoved()); - Assertions.assertEquals(5, orderedDisharmony3.getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(2, orderedDisharmony3.getPackageCycleCount()); RankedDisharmony orderedDisharmony4 = disharmonies.get(4); Assertions.assertEquals("Class4", orderedDisharmony4.getClassName()); @@ -338,7 +403,7 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { Assertions.assertEquals(3, orderedDisharmony4.getCycleCount().intValue()); Assertions.assertEquals(0, orderedDisharmony4.getSourceNodeShouldBeRemoved()); Assertions.assertEquals(0, orderedDisharmony4.getTargetNodeShouldBeRemoved()); - Assertions.assertEquals(6, orderedDisharmony4.getEdgeSourceDisharmonyCount()); + Assertions.assertEquals(0, orderedDisharmony4.getPackageCycleCount()); } private void writeFile(String name, String content) throws IOException { diff --git a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java index 6d1a5f1..3c3501b 100644 --- a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java +++ b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java @@ -302,10 +302,10 @@ public StringBuilder generateReport( List packageRelationshipDisharmonies = List.of(); try (CostBenefitCalculator costBenefitCalculator = new CostBenefitCalculator(projectBaseDir, codebaseGraphDTO.getClassToSourceFilePathMapping())) { - classRelationshipDisharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - classGraph, classEdgeCycleCounts, codebaseGraphDTO, classesToRemove); packageRelationshipDisharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - packageGraph, packageEdgeCycleCounts, codebaseGraphDTO, packagesToRemove); + packageGraph, packageEdgeCycleCounts, codebaseGraphDTO, packagesToRemove, packageCycles); + classRelationshipDisharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( + classGraph, classEdgeCycleCounts, codebaseGraphDTO, classesToRemove, packageCycles); for (DisharmonySpec spec : disharmonySpecs) { List instances = spec.methodLevel() @@ -316,7 +316,6 @@ public StringBuilder generateReport( spec.anchorId(), costBenefitCalculator.calculateDisharmonyCostBenefitValues(instances)); } } - } catch (Exception e) { log.error("Error running analysis."); throw new RuntimeException(e); @@ -358,7 +357,8 @@ public StringBuilder generateReport( stringBuilder.append("
\n"); if (!classRelationshipDisharmonies.isEmpty()) { - stringBuilder.append(renderClassEdgeDisharmonies(classRelationshipDisharmonies, repoUrl, codebaseGraphDTO)); + stringBuilder.append(renderClassEdgeDisharmonies( + classRelationshipDisharmonies, packageRelationshipDisharmonies, repoUrl, codebaseGraphDTO)); stringBuilder.append("
\n" + "
\n" + "
\n" + "
\n" + "
\n" + "
\n" + "
\n"); } @@ -467,7 +467,10 @@ private String renderCycles(List rankedCycles, String repoUrl, Code } private String renderClassEdgeDisharmonies( - List edgeDisharmonies, String repoUrl, CodebaseGraphDTO codebaseGraphDTO) { + List classRelationshipDisharmonies, + List packageRelationshipDisharmonies, + String repoUrl, + CodebaseGraphDTO codebaseGraphDTO) { StringBuilder stringBuilder = new StringBuilder(); stringBuilder.append( @@ -499,7 +502,7 @@ private String renderClassEdgeDisharmonies( stringBuilder.append("\n"); - for (RankedDisharmony edge : edgeDisharmonies) { + for (RankedDisharmony edge : classRelationshipDisharmonies) { stringBuilder.append("\n"); for (String rowData : getClassRelationshipDisharmony(edge, repoUrl, codebaseGraphDTO)) { @@ -568,12 +571,7 @@ private String renderPackageEdgeDisharmonies( private String[] getClassRelationshipDisharmonyTableHeadings() { return new String[] { - "Relationship", - "Priority", - "In Cycles", - "Edge
Weight", - "Source
Disharmony Count", - "Target
Disharmony Count", + "Relationship", "Priority", "In Class
Cycles", "Relationship
Strength", "In Package
Cycles", }; } @@ -590,8 +588,7 @@ private String[] getClassRelationshipDisharmony( String.valueOf(edgeInfo.getPriority()), String.valueOf(edgeInfo.getCycleCount()), String.valueOf(edgeInfo.getEffortRank()), - String.valueOf(edgeInfo.getEdgeSourceDisharmonyCount()), - String.valueOf(edgeInfo.getEdgeTargetDisharmonyCount()), + String.valueOf(edgeInfo.getPackageCycleCount()), }; } From 852ec5950f69deed3171daa888683a01c6325076 Mon Sep 17 00:00:00 2001 From: Jim Bethancourt Date: Tue, 30 Jun 2026 20:41:17 -0500 Subject: [PATCH 2/2] Indicating if the class relationship to be removed also contributes to a package relationship cycle --- .../org/hjug/cbc/CostBenefitCalculator.java | 28 ++++- .../java/org/hjug/cbc/RankedDisharmony.java | 5 +- .../hjug/cbc/CostBenefitCalculatorTest.java | 119 ++++++++++++++---- .../report/SimpleHtmlReport.java | 18 ++- 4 files changed, 137 insertions(+), 33 deletions(-) diff --git a/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java b/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java index 175824a..eefa08f 100644 --- a/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java +++ b/cost-benefit-calculator/src/main/java/org/hjug/cbc/CostBenefitCalculator.java @@ -332,9 +332,14 @@ public List calculateRelationshipCostBenefitValues( Map edgeToRemoveCycleCounts, CodebaseGraphDTO dto, Set vertexesToRemove, - Map> packageCycles) { + Map> packageCycles, + List packageRelationshipDisharmonies) { List edgesThatNeedToBeRemoved = new ArrayList<>(); + Set 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; @@ -352,7 +357,8 @@ public List calculateRelationshipCostBenefitValues( (int) classGraph.getEdgeWeight(edge), sourceNodeShouldBeRemoved, targetNodeShouldBeRemoved, - getPackageCycleCount(edgeSource, edgeTarget, dto, packageCycles)); + getPackageCycleCount(edgeSource, edgeTarget, dto, packageCycles), + packageRelationshipShouldBeRemoved(edgeSource, edgeTarget, dto, packageEdgesToRemove)); edgesThatNeedToBeRemoved.add(edgeThatNeedsToBeRemoved); } @@ -399,6 +405,19 @@ private static int getPackageCycleCount( 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 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. @@ -420,8 +439,11 @@ static void sortEdgesThatNeedToBeRemoved(List rankedDisharmoni .reversed() // then by weight, with lowest weight edges bubbling to the top .thenComparingInt(RankedDisharmony::getEffortRank) - // then by package cycle count, with classes in more package cycles bubbling to the top + // 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()) diff --git a/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java b/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java index 4a5a072..38547f4 100644 --- a/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java +++ b/cost-benefit-calculator/src/main/java/org/hjug/cbc/RankedDisharmony.java @@ -45,6 +45,7 @@ public class RankedDisharmony { private int targetNodeShouldBeRemoved; private String edgeTargetClass; private Integer packageCycleCount; + private boolean packageRelationshipShouldBeRemoved; public RankedDisharmony(GodClass godClass, ScmLogInfo scmLogInfo) { path = scmLogInfo.getPath(); @@ -108,12 +109,14 @@ public RankedDisharmony( int weight, boolean sourceNodeShouldBeRemoved, boolean targetNodeShouldBeRemoved, - int packageCycleCount) { + int packageCycleCount, + boolean packageRelationshipShouldBeRemoved) { className = edgeSource; this.edge = edge; this.cycleCount = cycleCount; this.packageCycleCount = packageCycleCount; + this.packageRelationshipShouldBeRemoved = packageRelationshipShouldBeRemoved; effortRank = weight; this.sourceNodeShouldBeRemoved = sourceNodeShouldBeRemoved ? 1 : 0; this.targetNodeShouldBeRemoved = targetNodeShouldBeRemoved ? 1 : 0; diff --git a/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java b/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java index 10467bf..058b729 100644 --- a/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java +++ b/cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java @@ -158,7 +158,7 @@ void calculateRelationshipCostBenefitValues_filtersMissingLogInfoAndAssignsPrior .thenReturn(new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class)); List disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap()); + classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap(), List.of()); Assertions.assertEquals(2, disharmonies.size()); @@ -231,7 +231,7 @@ void calculateRelationshipCostBenefitValues_prefersHigherChangePronenessRank() t .thenReturn(new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class)); List disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap()); + classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap(), List.of()); Assertions.assertEquals(2, disharmonies.size()); Assertions.assertEquals(0, disharmonies.get(0).getPackageCycleCount()); @@ -289,7 +289,7 @@ void calculateRelationshipCostBenefitValues_packageCycleCountReflectsRelationshi new CostBenefitCalculator(git.getRepository().getDirectory().getParent(), new HashMap<>())) { List disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - classGraph, edgeToRemoveCycleCounts, dto, Collections.emptySet(), packageCycles); + classGraph, edgeToRemoveCycleCounts, dto, Collections.emptySet(), packageCycles, List.of()); Map disharmoniesByEdge = new HashMap<>(); disharmonies.forEach(d -> disharmoniesByEdge.put(d.getEdge(), d)); @@ -304,6 +304,68 @@ void calculateRelationshipCostBenefitValues_packageCycleCountReflectsRelationshi } } + @Test + void calculateRelationshipCostBenefitValues_flagsClassEdgesWhosePackageRelationshipIsAlsoBeingRemoved() + throws Exception { + + writeFile(hudsonPath + "Placeholder.java", "public class Placeholder {}"); + git.add().addFilepattern(".").call(); + git.commit().setMessage("initial commit").call(); + + SimpleDirectedWeightedGraph packageGraph = + new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class); + packageGraph.addVertex("pkga"); + packageGraph.addVertex("pkgb"); + packageGraph.addVertex("pkgc"); + DefaultWeightedEdge pkgaToPkgb = packageGraph.addEdge("pkga", "pkgb"); + packageGraph.addEdge("pkgb", "pkgc"); + + // Only pkga -> pkgb is flagged as a package relationship that needs to be removed + RankedDisharmony packageEdgeToRemove = new RankedDisharmony("pkga", pkgaToPkgb, 1, 1, false, false, 0, false); + List packageRelationshipDisharmonies = List.of(packageEdgeToRemove); + + SimpleDirectedWeightedGraph classGraph = + new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class); + classGraph.addVertex("pkga.Foo"); + classGraph.addVertex("pkgb.Bar"); + classGraph.addVertex("pkgc.Baz"); + classGraph.addVertex("pkga.Other"); + + // matches the removed package relationship pkga -> pkgb + DefaultWeightedEdge fooToBar = classGraph.addEdge("pkga.Foo", "pkgb.Bar"); + // corresponds to pkgb -> pkgc, which is not in packageRelationshipDisharmonies + DefaultWeightedEdge barToBaz = classGraph.addEdge("pkgb.Bar", "pkgc.Baz"); + // same-package relationship: no package edge exists at all + DefaultWeightedEdge fooToOther = classGraph.addEdge("pkga.Foo", "pkga.Other"); + + Map edgeToRemoveCycleCounts = new HashMap<>(); + edgeToRemoveCycleCounts.put(fooToBar, 1); + edgeToRemoveCycleCounts.put(barToBaz, 1); + edgeToRemoveCycleCounts.put(fooToOther, 1); + + CodebaseGraphDTO dto = mock(CodebaseGraphDTO.class); + when(dto.getPackageReferencesGraph()).thenReturn(packageGraph); + + try (CostBenefitCalculator costBenefitCalculator = + new CostBenefitCalculator(git.getRepository().getDirectory().getParent(), new HashMap<>())) { + + List disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( + classGraph, + edgeToRemoveCycleCounts, + dto, + Collections.emptySet(), + Collections.emptyMap(), + packageRelationshipDisharmonies); + + Map disharmoniesByEdge = new HashMap<>(); + disharmonies.forEach(d -> disharmoniesByEdge.put(d.getEdge(), d)); + + Assertions.assertTrue(disharmoniesByEdge.get(fooToBar).isPackageRelationshipShouldBeRemoved()); + Assertions.assertFalse(disharmoniesByEdge.get(barToBaz).isPackageRelationshipShouldBeRemoved()); + Assertions.assertFalse(disharmoniesByEdge.get(fooToOther).isPackageRelationshipShouldBeRemoved()); + } + } + @Test void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { // Create ScmLogInfo objects for testing @@ -323,27 +385,30 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { logInfo5.setChangePronenessRank(5); // Create RankedDisharmony objects with different combinations - // Expected order after sorting: cycleCount desc, then effortRank asc, then packageCycleCount desc, + // Expected order after sorting: cycleCount desc, then effortRank asc, + // then packageRelationshipShouldBeRemoved desc (true before false), then packageCycleCount desc, // then sourceRemoved desc, then targetRemoved desc - // cycle=5, source=0, target=0, packageCycleCount=6 - RankedDisharmony disharmony1 = - new RankedDisharmony("Class1", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, false, false, 6); + // cycle=5, source=0, target=0, packageCycleCount=6, packageRelationshipShouldBeRemoved=false + RankedDisharmony disharmony1 = new RankedDisharmony( + "Class1", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, false, false, 6, false); - // cycle=5, source=1, target=0, packageCycleCount=1 - RankedDisharmony disharmony2 = - new RankedDisharmony("Class2", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, true, false, 1); + // cycle=5, source=1, target=0, packageCycleCount=1, packageRelationshipShouldBeRemoved=false + RankedDisharmony disharmony2 = new RankedDisharmony( + "Class2", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, true, false, 1, false); - // cycle=3, source=0, target=1, packageCycleCount=5 - RankedDisharmony disharmony3 = - new RankedDisharmony("Class3", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, true, 5); + // cycle=3, source=0, target=1, packageCycleCount=5, packageRelationshipShouldBeRemoved=false + RankedDisharmony disharmony3 = new RankedDisharmony( + "Class3", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, true, 5, false); - // cycle=3, source=0, target=0, packageCycleCount=0 - RankedDisharmony disharmony4 = - new RankedDisharmony("Class4", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 0); + // cycle=3, source=0, target=0, packageCycleCount=0, packageRelationshipShouldBeRemoved=false + RankedDisharmony disharmony4 = new RankedDisharmony( + "Class4", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 0, false); - // cycle=3, source=0, target=0, packageCycleCount=2 - RankedDisharmony disharmony5 = - new RankedDisharmony("Class5", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 2); + // cycle=3, source=0, target=0, packageCycleCount=2, packageRelationshipShouldBeRemoved=true + // lower packageCycleCount than disharmony3, but packageRelationshipShouldBeRemoved=true must still + // bubble it ahead of disharmony3, proving the new sort clause is applied before packageCycleCount + RankedDisharmony disharmony5 = new RankedDisharmony( + "Class5", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 2, true); List disharmonies = Arrays.asList(disharmony4, disharmony2, disharmony1, disharmony3, disharmony5); @@ -381,21 +446,23 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() { Assertions.assertEquals(0, orderedDisharmony1.getTargetNodeShouldBeRemoved()); Assertions.assertEquals(1, orderedDisharmony1.getPackageCycleCount()); + // Class5 has a lower packageCycleCount than Class3, but packageRelationshipShouldBeRemoved=true + // outranks packageCycleCount, so it bubbles ahead RankedDisharmony orderedDisharmony2 = disharmonies.get(2); - Assertions.assertEquals("Class3", orderedDisharmony2.getClassName()); + Assertions.assertEquals("Class5", orderedDisharmony2.getClassName()); Assertions.assertEquals(3, orderedDisharmony2.getCycleCount().intValue()); Assertions.assertEquals(1, orderedDisharmony2.getEffortRank().intValue()); - Assertions.assertEquals(0, orderedDisharmony2.getSourceNodeShouldBeRemoved()); - Assertions.assertEquals(1, orderedDisharmony2.getTargetNodeShouldBeRemoved()); - Assertions.assertEquals(5, orderedDisharmony2.getPackageCycleCount()); + Assertions.assertTrue(orderedDisharmony2.isPackageRelationshipShouldBeRemoved()); + Assertions.assertEquals(2, orderedDisharmony2.getPackageCycleCount()); RankedDisharmony orderedDisharmony3 = disharmonies.get(3); - Assertions.assertEquals("Class5", orderedDisharmony3.getClassName()); + Assertions.assertEquals("Class3", orderedDisharmony3.getClassName()); Assertions.assertEquals(3, orderedDisharmony3.getCycleCount().intValue()); Assertions.assertEquals(1, orderedDisharmony3.getEffortRank().intValue()); Assertions.assertEquals(0, orderedDisharmony3.getSourceNodeShouldBeRemoved()); - Assertions.assertEquals(0, orderedDisharmony3.getTargetNodeShouldBeRemoved()); - Assertions.assertEquals(2, orderedDisharmony3.getPackageCycleCount()); + Assertions.assertEquals(1, orderedDisharmony3.getTargetNodeShouldBeRemoved()); + Assertions.assertFalse(orderedDisharmony3.isPackageRelationshipShouldBeRemoved()); + Assertions.assertEquals(5, orderedDisharmony3.getPackageCycleCount()); RankedDisharmony orderedDisharmony4 = disharmonies.get(4); Assertions.assertEquals("Class4", orderedDisharmony4.getClassName()); diff --git a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java index 3c3501b..dd5bda2 100644 --- a/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java +++ b/report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java @@ -303,9 +303,14 @@ public StringBuilder generateReport( try (CostBenefitCalculator costBenefitCalculator = new CostBenefitCalculator(projectBaseDir, codebaseGraphDTO.getClassToSourceFilePathMapping())) { packageRelationshipDisharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - packageGraph, packageEdgeCycleCounts, codebaseGraphDTO, packagesToRemove, packageCycles); + packageGraph, packageEdgeCycleCounts, codebaseGraphDTO, packagesToRemove, packageCycles, List.of()); classRelationshipDisharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues( - classGraph, classEdgeCycleCounts, codebaseGraphDTO, classesToRemove, packageCycles); + classGraph, + classEdgeCycleCounts, + codebaseGraphDTO, + classesToRemove, + packageCycles, + packageRelationshipDisharmonies); for (DisharmonySpec spec : disharmonySpecs) { List instances = spec.methodLevel() @@ -571,7 +576,12 @@ private String renderPackageEdgeDisharmonies( private String[] getClassRelationshipDisharmonyTableHeadings() { return new String[] { - "Relationship", "Priority", "In Class
Cycles", "Relationship
Strength", "In Package
Cycles", + "Relationship", + "Priority", + "In Class
Cycles", + "Relationship
Strength", + "Also Remove
Package Relationship", + "In Package
Cycles", }; } @@ -583,11 +593,13 @@ private String[] getPackageRelationshipDisharmonyTableHeadings() { private String[] getClassRelationshipDisharmony( RankedDisharmony edgeInfo, String repoUrl, CodebaseGraphDTO codebaseGraphDTO) { + boolean removePkgRel = edgeInfo.isPackageRelationshipShouldBeRemoved(); return new String[] { renderClassEdge(edgeInfo.getEdge(), repoUrl, codebaseGraphDTO), String.valueOf(edgeInfo.getPriority()), String.valueOf(edgeInfo.getCycleCount()), String.valueOf(edgeInfo.getEffortRank()), + removePkgRel ? "true" : String.valueOf(false), String.valueOf(edgeInfo.getPackageCycleCount()), }; }