Skip to content

Commit 852ec59

Browse files
jimbethancourtjimbethancourt
committed
Indicating if the class relationship to be removed also contributes to a package relationship cycle
1 parent c64d25a commit 852ec59

4 files changed

Lines changed: 137 additions & 33 deletions

File tree

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

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -332,9 +332,14 @@ public List<RankedDisharmony> calculateRelationshipCostBenefitValues(
332332
Map<DefaultWeightedEdge, Integer> edgeToRemoveCycleCounts,
333333
CodebaseGraphDTO dto,
334334
Set<String> vertexesToRemove,
335-
Map<String, AsSubgraph<String, DefaultWeightedEdge>> packageCycles) {
335+
Map<String, AsSubgraph<String, DefaultWeightedEdge>> packageCycles,
336+
List<RankedDisharmony> packageRelationshipDisharmonies) {
336337
List<RankedDisharmony> edgesThatNeedToBeRemoved = new ArrayList<>();
337338

339+
Set<DefaultWeightedEdge> packageEdgesToRemove = packageRelationshipDisharmonies.stream()
340+
.map(RankedDisharmony::getEdge)
341+
.collect(Collectors.toSet());
342+
338343
for (DefaultWeightedEdge edge : classGraph.edgeSet()) {
339344
// shouldn't have to check for null edges & counts :-(
340345
if (null == edge || null == edgeToRemoveCycleCounts.get(edge)) continue;
@@ -352,7 +357,8 @@ public List<RankedDisharmony> calculateRelationshipCostBenefitValues(
352357
(int) classGraph.getEdgeWeight(edge),
353358
sourceNodeShouldBeRemoved,
354359
targetNodeShouldBeRemoved,
355-
getPackageCycleCount(edgeSource, edgeTarget, dto, packageCycles));
360+
getPackageCycleCount(edgeSource, edgeTarget, dto, packageCycles),
361+
packageRelationshipShouldBeRemoved(edgeSource, edgeTarget, dto, packageEdgesToRemove));
356362

357363
edgesThatNeedToBeRemoved.add(edgeThatNeedsToBeRemoved);
358364
}
@@ -399,6 +405,19 @@ private static int getPackageCycleCount(
399405
return packageCycleCount;
400406
}
401407

408+
/**
409+
* Determines whether the package-level relationship corresponding to the given class (or package) edge is
410+
* itself one of the package edges selected for removal, based on membership in the already-computed package
411+
* relationship disharmonies.
412+
*/
413+
private static boolean packageRelationshipShouldBeRemoved(
414+
String edgeSource, String edgeTarget, CodebaseGraphDTO dto, Set<DefaultWeightedEdge> packageEdgesToRemove) {
415+
String sourcePackage = toPackageName(edgeSource, dto);
416+
String targetPackage = toPackageName(edgeTarget, dto);
417+
DefaultWeightedEdge packageEdge = dto.getPackageReferencesGraph().getEdge(sourcePackage, targetPackage);
418+
return packageEdge != null && packageEdgesToRemove.contains(packageEdge);
419+
}
420+
402421
/**
403422
* The vertex may already be a package name (when classGraph is actually a package graph) or a fully-qualified
404423
* class name, in which case the containing package is derived from it.
@@ -420,8 +439,11 @@ static void sortEdgesThatNeedToBeRemoved(List<RankedDisharmony> rankedDisharmoni
420439
.reversed()
421440
// then by weight, with lowest weight edges bubbling to the top
422441
.thenComparingInt(RankedDisharmony::getEffortRank)
423-
// then by package cycle count, with classes in more package cycles bubbling to the top
442+
// then by whether the underlying package relationship should also be removed, true before false
424443
// multiplying by -1 reverses the sort order (reverse doesn't work in chained comparators)
444+
.thenComparingInt(
445+
rankedDisharmony -> -1 * (rankedDisharmony.isPackageRelationshipShouldBeRemoved() ? 1 : 0))
446+
// then by package cycle count, with classes in more package cycles bubbling to the top
425447
.thenComparingInt(rankedDisharmony -> -1 * rankedDisharmony.getPackageCycleCount())
426448
// then if the source node is in the list of nodes to be removed
427449
.thenComparingInt(rankedDisharmony -> -1 * rankedDisharmony.getSourceNodeShouldBeRemoved())

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ public class RankedDisharmony {
4545
private int targetNodeShouldBeRemoved;
4646
private String edgeTargetClass;
4747
private Integer packageCycleCount;
48+
private boolean packageRelationshipShouldBeRemoved;
4849

4950
public RankedDisharmony(GodClass godClass, ScmLogInfo scmLogInfo) {
5051
path = scmLogInfo.getPath();
@@ -108,12 +109,14 @@ public RankedDisharmony(
108109
int weight,
109110
boolean sourceNodeShouldBeRemoved,
110111
boolean targetNodeShouldBeRemoved,
111-
int packageCycleCount) {
112+
int packageCycleCount,
113+
boolean packageRelationshipShouldBeRemoved) {
112114

113115
className = edgeSource;
114116
this.edge = edge;
115117
this.cycleCount = cycleCount;
116118
this.packageCycleCount = packageCycleCount;
119+
this.packageRelationshipShouldBeRemoved = packageRelationshipShouldBeRemoved;
117120
effortRank = weight;
118121
this.sourceNodeShouldBeRemoved = sourceNodeShouldBeRemoved ? 1 : 0;
119122
this.targetNodeShouldBeRemoved = targetNodeShouldBeRemoved ? 1 : 0;

‎cost-benefit-calculator/src/test/java/org/hjug/cbc/CostBenefitCalculatorTest.java‎

Lines changed: 93 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ void calculateRelationshipCostBenefitValues_filtersMissingLogInfoAndAssignsPrior
158158
.thenReturn(new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class));
159159

160160
List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues(
161-
classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap());
161+
classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap(), List.of());
162162

163163
Assertions.assertEquals(2, disharmonies.size());
164164

@@ -231,7 +231,7 @@ void calculateRelationshipCostBenefitValues_prefersHigherChangePronenessRank() t
231231
.thenReturn(new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class));
232232

233233
List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues(
234-
classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap());
234+
classGraph, edgeToRemoveCycleCounts, dto, vertexesToRemove, Collections.emptyMap(), List.of());
235235

236236
Assertions.assertEquals(2, disharmonies.size());
237237
Assertions.assertEquals(0, disharmonies.get(0).getPackageCycleCount());
@@ -289,7 +289,7 @@ void calculateRelationshipCostBenefitValues_packageCycleCountReflectsRelationshi
289289
new CostBenefitCalculator(git.getRepository().getDirectory().getParent(), new HashMap<>())) {
290290

291291
List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues(
292-
classGraph, edgeToRemoveCycleCounts, dto, Collections.emptySet(), packageCycles);
292+
classGraph, edgeToRemoveCycleCounts, dto, Collections.emptySet(), packageCycles, List.of());
293293

294294
Map<DefaultWeightedEdge, RankedDisharmony> disharmoniesByEdge = new HashMap<>();
295295
disharmonies.forEach(d -> disharmoniesByEdge.put(d.getEdge(), d));
@@ -304,6 +304,68 @@ void calculateRelationshipCostBenefitValues_packageCycleCountReflectsRelationshi
304304
}
305305
}
306306

307+
@Test
308+
void calculateRelationshipCostBenefitValues_flagsClassEdgesWhosePackageRelationshipIsAlsoBeingRemoved()
309+
throws Exception {
310+
311+
writeFile(hudsonPath + "Placeholder.java", "public class Placeholder {}");
312+
git.add().addFilepattern(".").call();
313+
git.commit().setMessage("initial commit").call();
314+
315+
SimpleDirectedWeightedGraph<String, DefaultWeightedEdge> packageGraph =
316+
new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class);
317+
packageGraph.addVertex("pkga");
318+
packageGraph.addVertex("pkgb");
319+
packageGraph.addVertex("pkgc");
320+
DefaultWeightedEdge pkgaToPkgb = packageGraph.addEdge("pkga", "pkgb");
321+
packageGraph.addEdge("pkgb", "pkgc");
322+
323+
// Only pkga -> pkgb is flagged as a package relationship that needs to be removed
324+
RankedDisharmony packageEdgeToRemove = new RankedDisharmony("pkga", pkgaToPkgb, 1, 1, false, false, 0, false);
325+
List<RankedDisharmony> packageRelationshipDisharmonies = List.of(packageEdgeToRemove);
326+
327+
SimpleDirectedWeightedGraph<String, DefaultWeightedEdge> classGraph =
328+
new SimpleDirectedWeightedGraph<>(DefaultWeightedEdge.class);
329+
classGraph.addVertex("pkga.Foo");
330+
classGraph.addVertex("pkgb.Bar");
331+
classGraph.addVertex("pkgc.Baz");
332+
classGraph.addVertex("pkga.Other");
333+
334+
// matches the removed package relationship pkga -> pkgb
335+
DefaultWeightedEdge fooToBar = classGraph.addEdge("pkga.Foo", "pkgb.Bar");
336+
// corresponds to pkgb -> pkgc, which is not in packageRelationshipDisharmonies
337+
DefaultWeightedEdge barToBaz = classGraph.addEdge("pkgb.Bar", "pkgc.Baz");
338+
// same-package relationship: no package edge exists at all
339+
DefaultWeightedEdge fooToOther = classGraph.addEdge("pkga.Foo", "pkga.Other");
340+
341+
Map<DefaultWeightedEdge, Integer> edgeToRemoveCycleCounts = new HashMap<>();
342+
edgeToRemoveCycleCounts.put(fooToBar, 1);
343+
edgeToRemoveCycleCounts.put(barToBaz, 1);
344+
edgeToRemoveCycleCounts.put(fooToOther, 1);
345+
346+
CodebaseGraphDTO dto = mock(CodebaseGraphDTO.class);
347+
when(dto.getPackageReferencesGraph()).thenReturn(packageGraph);
348+
349+
try (CostBenefitCalculator costBenefitCalculator =
350+
new CostBenefitCalculator(git.getRepository().getDirectory().getParent(), new HashMap<>())) {
351+
352+
List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues(
353+
classGraph,
354+
edgeToRemoveCycleCounts,
355+
dto,
356+
Collections.emptySet(),
357+
Collections.emptyMap(),
358+
packageRelationshipDisharmonies);
359+
360+
Map<DefaultWeightedEdge, RankedDisharmony> disharmoniesByEdge = new HashMap<>();
361+
disharmonies.forEach(d -> disharmoniesByEdge.put(d.getEdge(), d));
362+
363+
Assertions.assertTrue(disharmoniesByEdge.get(fooToBar).isPackageRelationshipShouldBeRemoved());
364+
Assertions.assertFalse(disharmoniesByEdge.get(barToBaz).isPackageRelationshipShouldBeRemoved());
365+
Assertions.assertFalse(disharmoniesByEdge.get(fooToOther).isPackageRelationshipShouldBeRemoved());
366+
}
367+
}
368+
307369
@Test
308370
void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() {
309371
// Create ScmLogInfo objects for testing
@@ -323,27 +385,30 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() {
323385
logInfo5.setChangePronenessRank(5);
324386

325387
// Create RankedDisharmony objects with different combinations
326-
// Expected order after sorting: cycleCount desc, then effortRank asc, then packageCycleCount desc,
388+
// Expected order after sorting: cycleCount desc, then effortRank asc,
389+
// then packageRelationshipShouldBeRemoved desc (true before false), then packageCycleCount desc,
327390
// then sourceRemoved desc, then targetRemoved desc
328-
// cycle=5, source=0, target=0, packageCycleCount=6
329-
RankedDisharmony disharmony1 =
330-
new RankedDisharmony("Class1", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, false, false, 6);
391+
// cycle=5, source=0, target=0, packageCycleCount=6, packageRelationshipShouldBeRemoved=false
392+
RankedDisharmony disharmony1 = new RankedDisharmony(
393+
"Class1", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, false, false, 6, false);
331394

332-
// cycle=5, source=1, target=0, packageCycleCount=1
333-
RankedDisharmony disharmony2 =
334-
new RankedDisharmony("Class2", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, true, false, 1);
395+
// cycle=5, source=1, target=0, packageCycleCount=1, packageRelationshipShouldBeRemoved=false
396+
RankedDisharmony disharmony2 = new RankedDisharmony(
397+
"Class2", new org.jgrapht.graph.DefaultWeightedEdge(), 5, 1, true, false, 1, false);
335398

336-
// cycle=3, source=0, target=1, packageCycleCount=5
337-
RankedDisharmony disharmony3 =
338-
new RankedDisharmony("Class3", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, true, 5);
399+
// cycle=3, source=0, target=1, packageCycleCount=5, packageRelationshipShouldBeRemoved=false
400+
RankedDisharmony disharmony3 = new RankedDisharmony(
401+
"Class3", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, true, 5, false);
339402

340-
// cycle=3, source=0, target=0, packageCycleCount=0
341-
RankedDisharmony disharmony4 =
342-
new RankedDisharmony("Class4", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 0);
403+
// cycle=3, source=0, target=0, packageCycleCount=0, packageRelationshipShouldBeRemoved=false
404+
RankedDisharmony disharmony4 = new RankedDisharmony(
405+
"Class4", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 0, false);
343406

344-
// cycle=3, source=0, target=0, packageCycleCount=2
345-
RankedDisharmony disharmony5 =
346-
new RankedDisharmony("Class5", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 2);
407+
// cycle=3, source=0, target=0, packageCycleCount=2, packageRelationshipShouldBeRemoved=true
408+
// lower packageCycleCount than disharmony3, but packageRelationshipShouldBeRemoved=true must still
409+
// bubble it ahead of disharmony3, proving the new sort clause is applied before packageCycleCount
410+
RankedDisharmony disharmony5 = new RankedDisharmony(
411+
"Class5", new org.jgrapht.graph.DefaultWeightedEdge(), 3, 1, false, false, 2, true);
347412

348413
List<RankedDisharmony> disharmonies =
349414
Arrays.asList(disharmony4, disharmony2, disharmony1, disharmony3, disharmony5);
@@ -381,21 +446,23 @@ void sortEdgesThatNeedToBeRemoved_sortsByMultipleCriteria() {
381446
Assertions.assertEquals(0, orderedDisharmony1.getTargetNodeShouldBeRemoved());
382447
Assertions.assertEquals(1, orderedDisharmony1.getPackageCycleCount());
383448

449+
// Class5 has a lower packageCycleCount than Class3, but packageRelationshipShouldBeRemoved=true
450+
// outranks packageCycleCount, so it bubbles ahead
384451
RankedDisharmony orderedDisharmony2 = disharmonies.get(2);
385-
Assertions.assertEquals("Class3", orderedDisharmony2.getClassName());
452+
Assertions.assertEquals("Class5", orderedDisharmony2.getClassName());
386453
Assertions.assertEquals(3, orderedDisharmony2.getCycleCount().intValue());
387454
Assertions.assertEquals(1, orderedDisharmony2.getEffortRank().intValue());
388-
Assertions.assertEquals(0, orderedDisharmony2.getSourceNodeShouldBeRemoved());
389-
Assertions.assertEquals(1, orderedDisharmony2.getTargetNodeShouldBeRemoved());
390-
Assertions.assertEquals(5, orderedDisharmony2.getPackageCycleCount());
455+
Assertions.assertTrue(orderedDisharmony2.isPackageRelationshipShouldBeRemoved());
456+
Assertions.assertEquals(2, orderedDisharmony2.getPackageCycleCount());
391457

392458
RankedDisharmony orderedDisharmony3 = disharmonies.get(3);
393-
Assertions.assertEquals("Class5", orderedDisharmony3.getClassName());
459+
Assertions.assertEquals("Class3", orderedDisharmony3.getClassName());
394460
Assertions.assertEquals(3, orderedDisharmony3.getCycleCount().intValue());
395461
Assertions.assertEquals(1, orderedDisharmony3.getEffortRank().intValue());
396462
Assertions.assertEquals(0, orderedDisharmony3.getSourceNodeShouldBeRemoved());
397-
Assertions.assertEquals(0, orderedDisharmony3.getTargetNodeShouldBeRemoved());
398-
Assertions.assertEquals(2, orderedDisharmony3.getPackageCycleCount());
463+
Assertions.assertEquals(1, orderedDisharmony3.getTargetNodeShouldBeRemoved());
464+
Assertions.assertFalse(orderedDisharmony3.isPackageRelationshipShouldBeRemoved());
465+
Assertions.assertEquals(5, orderedDisharmony3.getPackageCycleCount());
399466

400467
RankedDisharmony orderedDisharmony4 = disharmonies.get(4);
401468
Assertions.assertEquals("Class4", orderedDisharmony4.getClassName());

‎report/src/main/java/org/hjug/refactorfirst/report/SimpleHtmlReport.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -303,9 +303,14 @@ public StringBuilder generateReport(
303303
try (CostBenefitCalculator costBenefitCalculator =
304304
new CostBenefitCalculator(projectBaseDir, codebaseGraphDTO.getClassToSourceFilePathMapping())) {
305305
packageRelationshipDisharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues(
306-
packageGraph, packageEdgeCycleCounts, codebaseGraphDTO, packagesToRemove, packageCycles);
306+
packageGraph, packageEdgeCycleCounts, codebaseGraphDTO, packagesToRemove, packageCycles, List.of());
307307
classRelationshipDisharmonies = costBenefitCalculator.calculateRelationshipCostBenefitValues(
308-
classGraph, classEdgeCycleCounts, codebaseGraphDTO, classesToRemove, packageCycles);
308+
classGraph,
309+
classEdgeCycleCounts,
310+
codebaseGraphDTO,
311+
classesToRemove,
312+
packageCycles,
313+
packageRelationshipDisharmonies);
309314

310315
for (DisharmonySpec spec : disharmonySpecs) {
311316
List<DisharmonyInstance> instances = spec.methodLevel()
@@ -571,7 +576,12 @@ private String renderPackageEdgeDisharmonies(
571576

572577
private String[] getClassRelationshipDisharmonyTableHeadings() {
573578
return new String[] {
574-
"Relationship", "Priority", "In Class<br>Cycles", "Relationship<br>Strength", "In Package<br>Cycles",
579+
"Relationship",
580+
"Priority",
581+
"In Class<br>Cycles",
582+
"Relationship<br>Strength",
583+
"Also Remove<br>Package Relationship",
584+
"In Package<br>Cycles",
575585
};
576586
}
577587

@@ -583,11 +593,13 @@ private String[] getPackageRelationshipDisharmonyTableHeadings() {
583593

584594
private String[] getClassRelationshipDisharmony(
585595
RankedDisharmony edgeInfo, String repoUrl, CodebaseGraphDTO codebaseGraphDTO) {
596+
boolean removePkgRel = edgeInfo.isPackageRelationshipShouldBeRemoved();
586597
return new String[] {
587598
renderClassEdge(edgeInfo.getEdge(), repoUrl, codebaseGraphDTO),
588599
String.valueOf(edgeInfo.getPriority()),
589600
String.valueOf(edgeInfo.getCycleCount()),
590601
String.valueOf(edgeInfo.getEffortRank()),
602+
removePkgRel ? "<strong>true</strong>" : String.valueOf(false),
591603
String.valueOf(edgeInfo.getPackageCycleCount()),
592604
};
593605
}

0 commit comments

Comments
 (0)