Skip to content

Commit 8833c1b

Browse files
jimbethancourtjimbethancourt
committed
Add problem and solution details for disharmony types
1 parent 4e1caec commit 8833c1b

5 files changed

Lines changed: 187 additions & 91 deletions

File tree

‎codebase-graph-builder/src/main/java/org/hjug/graphbuilder/metrics/DisharmonyDetector.java‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -202,28 +202,6 @@ public List<MethodDisharmony> detectFeatureEnvy(List<ClassMetrics> allMetrics) {
202202
return featureEnvyMethods;
203203
}
204204

205-
// !Not used in production code for metric capture
206-
public List<MethodDisharmony> detectLongMethods(List<ClassMetrics> allMetrics) {
207-
List<MethodDisharmony> longMethods = new ArrayList<>();
208-
for (ClassMetrics classMetrics : allMetrics) {
209-
for (MethodMetrics methodMetrics : classMetrics.getMethods().values()) {
210-
if (isLongMethod(methodMetrics)) {
211-
String description = String.format("Long Method detected: LOC=%d", methodMetrics.getLinesOfCode());
212-
List<DisharmonyMetric> metricValues =
213-
List.of(new DisharmonyMetric("LOC", methodMetrics.getLinesOfCode(), Direction.ASCENDING));
214-
longMethods.add(new MethodDisharmony(
215-
classMetrics.getFullyQualifiedName(),
216-
methodMetrics.getSignature(),
217-
DisharmonyTypes.LONG_METHOD,
218-
description,
219-
methodMetrics,
220-
metricValues));
221-
}
222-
}
223-
}
224-
return longMethods;
225-
}
226-
227205
public List<MethodDisharmony> detectIntensiveCoupling(List<ClassMetrics> allMetrics) {
228206
List<MethodDisharmony> intensivelyCoupledMethods = new ArrayList<>();
229207
for (ClassMetrics classMetrics : allMetrics) {

‎codebase-graph-builder/src/main/java/org/hjug/graphbuilder/metrics/DisharmonyTypes.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ public final class DisharmonyTypes {
77
public static final String BRAIN_METHOD = "Brain Method";
88
public static final String BRAIN_CLASS = "Brain Class";
99
public static final String FEATURE_ENVY = "Feature Envy";
10-
public static final String LONG_METHOD = "Long Method";
1110
public static final String INTENSIVE_COUPLING = "Intensive Coupling";
1211
public static final String DISPERSED_COUPLING = "Dispersed Coupling";
1312
public static final String SHOTGUN_SURGERY = "Shotgun Surgery";

‎codebase-graph-builder/src/test/java/org/hjug/graphbuilder/metrics/DisharmonyDetectorMetricsTest.java‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -363,24 +363,6 @@ void featureEnvyDisharmonyEmitsStructuredMetrics() {
363363
assertEquals(Direction.DESCENDING, metrics.get(2).getDirection());
364364
}
365365

366-
// ── Long Method ────────────────────────────────────────────────────────────
367-
368-
@Test
369-
void longMethodDisharmonyEmitsStructuredMetrics() {
370-
ClassMetrics classMetrics = longMethodClassMetrics();
371-
allMetrics.add(classMetrics);
372-
373-
List<MethodDisharmony> result = detector.detectLongMethods(allMetrics);
374-
375-
assertFalse(result.isEmpty(), "should detect long method");
376-
List<DisharmonyMetric> metrics = result.get(0).getMetricValues();
377-
assertEquals(1, metrics.size());
378-
379-
assertEquals("LOC", metrics.get(0).getName());
380-
assertEquals(Direction.ASCENDING, metrics.get(0).getDirection());
381-
assertEquals(70.0, metrics.get(0).getValue());
382-
}
383-
384366
// ── Intensive Coupling ─────────────────────────────────────────────────────
385367

386368
@Test

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

Lines changed: 125 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -205,21 +205,94 @@ public StringBuilder generateReport(
205205

206206
// Ordered (type, anchorId, displayTitle, isMethodLevel) for all disharmonies
207207
final List<DisharmonySpec> disharmonySpecs = List.of(
208-
new DisharmonySpec(DisharmonyTypes.GOD_CLASS, "GOD", "God Classes", false),
209-
new DisharmonySpec(DisharmonyTypes.DATA_CLASS, "DATA_CLASS", "Data Classes", false),
210-
new DisharmonySpec(DisharmonyTypes.BRAIN_CLASS, "BRAIN_CLASS", "Brain Classes", false),
211-
new DisharmonySpec(DisharmonyTypes.REFUSED_PARENT_BEQUEST, "RPB", "Refused Parent Bequest", false),
212-
new DisharmonySpec(DisharmonyTypes.TRADITION_BREAKER, "TB", "Tradition Breakers", false),
213208
new DisharmonySpec(
214-
DisharmonyTypes.SIGNIFICANT_DUPLICATION, "SIG_DUP", "Significant Duplication", false),
215-
new DisharmonySpec(DisharmonyTypes.BRAIN_METHOD, "BRAIN_METHOD", "Brain Methods", true),
216-
new DisharmonySpec(DisharmonyTypes.FEATURE_ENVY, "FEATURE_ENVY", "Feature Envy", true),
217-
new DisharmonySpec(DisharmonyTypes.LONG_METHOD, "LONG_METHOD", "Long Methods", true),
209+
DisharmonyTypes.GOD_CLASS,
210+
"GOD",
211+
"God Classes",
212+
false,
213+
"God Classes take on too much responsibility,",
214+
"Extract related islands of functionality into separate classes. Leave God classes that don't change often alone."),
218215
new DisharmonySpec(
219-
DisharmonyTypes.INTENSIVE_COUPLING, "INTENSIVE_COUPLING", "Intensive Coupling", true),
216+
DisharmonyTypes.DATA_CLASS,
217+
"DATA_CLASS",
218+
"Data Classes",
219+
false,
220+
"Data Classes are dumb data holders that other classes rely on.",
221+
"Move the data/variable(s) to the same class as the operation."),
220222
new DisharmonySpec(
221-
DisharmonyTypes.DISPERSED_COUPLING, "DISPERSED_COUPLING", "Dispersed Coupling", true),
222-
new DisharmonySpec(DisharmonyTypes.SHOTGUN_SURGERY, "SHOTGUN_SURGERY", "Shotgun Surgery", true));
223+
DisharmonyTypes.BRAIN_CLASS,
224+
"BRAIN_CLASS",
225+
"Brain Classes",
226+
false,
227+
"Brain Classes are complex, lack cohesion, and have at least one Brain Method.",
228+
"Decompose Brain Methods into smaller methods."),
229+
new DisharmonySpec(
230+
DisharmonyTypes.REFUSED_PARENT_BEQUEST,
231+
"RPB",
232+
"Refused Parent Bequest",
233+
false,
234+
"Child class is large and often complex, but doesn't override or use many of the parent class's methods",
235+
"Do one or more of the following:<br>"
236+
+ "- Extract the child class into a separate class. Move the methods that are used from the parent class into the child class.<br>"
237+
+ "- Make unused protected members private in the parent class.<br>"
238+
+ "- If a parent class has multiple children, move methods not used by all descendants to another class."),
239+
new DisharmonySpec(
240+
DisharmonyTypes.TRADITION_BREAKER,
241+
"TB",
242+
"Tradition Breakers",
243+
false,
244+
"Child class adds many new public methods, but doesn't override or use many of the parent class's methods",
245+
"Do one or more of the following:<br>"
246+
+ "- Make public child methods unused outside of the class non-public.<br>"
247+
+ "- Pull duplicated methods in child classes into the parent class.<br>"
248+
+ "- Move methods in the child class that are unrelated to the parent class to another class.<br>"
249+
+ "- Remove the child class from the hierarchy."),
250+
new DisharmonySpec(
251+
DisharmonyTypes.SIGNIFICANT_DUPLICATION,
252+
"SIG_DUP",
253+
"Significant Duplication",
254+
false,
255+
"Nearly identical code is found in multiple classes, leading to increased maintenance costs.",
256+
"- Move duplicated code in the same class into a new method.<br>"
257+
+ "- Move duplicated code into a separate or parent class.<br>"
258+
+ "- Move duplicated code in two child classes or in parent/child classes into the parent class."),
259+
new DisharmonySpec(
260+
DisharmonyTypes.BRAIN_METHOD,
261+
"BRAIN_METHOD",
262+
"Brain Methods",
263+
true,
264+
"Method is long, complicated, and uses many variables.",
265+
"- Decompose the method into two or more smaller methods.<br>"
266+
+ "- If part of the method relies heavily on an outside class, extract that functionality out of the calling method and move it to the called class."),
267+
new DisharmonySpec(
268+
DisharmonyTypes.FEATURE_ENVY,
269+
"FEATURE_ENVY",
270+
"Feature Envy",
271+
true,
272+
"Method is more interested in data in other classes than its own class.",
273+
"Move the method (or part of the method) to the class where it uses the most data."),
274+
new DisharmonySpec(
275+
DisharmonyTypes.INTENSIVE_COUPLING,
276+
"INTENSIVE_COUPLING",
277+
"Intensive Coupling",
278+
true,
279+
"Method calls too many methods from a few unrelated classes (often in a separate package).",
280+
"Move the calling method to a class more closely related to the other classes that the original method can call."),
281+
new DisharmonySpec(
282+
DisharmonyTypes.DISPERSED_COUPLING,
283+
"DISPERSED_COUPLING",
284+
"Dispersed Coupling",
285+
true,
286+
"Method calls a few methods in many classes",
287+
"Reduce the size of the calling method. Extract methods from the calling method into the target classes."),
288+
new DisharmonySpec(
289+
DisharmonyTypes.SHOTGUN_SURGERY,
290+
"SHOTGUN_SURGERY",
291+
"Shotgun Surgery",
292+
true,
293+
"Method is called by many methods in many classes",
294+
"- Move the method closer to the calling classes (move the behavior closer to the data) if it is small.<br>"
295+
+ "- If it is a large method, treat is as a Brain Method and decompose it into two or more smaller methods."));
223296

224297
Map<String, List<RankedDisharmony>> rankedDisharmoniesByAnchor = new LinkedHashMap<>();
225298

@@ -303,8 +376,7 @@ public StringBuilder generateReport(
303376
for (DisharmonySpec spec : disharmonySpecs) {
304377
List<RankedDisharmony> rankedForType = rankedDisharmoniesByAnchor.get(spec.anchorId());
305378
if (rankedForType != null && !rankedForType.isEmpty()) {
306-
stringBuilder.append(renderDisharmonyInfo(
307-
repoUrl, spec.anchorId(), spec.title(), spec.methodLevel(), showDetails, rankedForType));
379+
stringBuilder.append(renderDisharmonyInfo(repoUrl, spec, showDetails, rankedForType));
308380
stringBuilder.append("<br/>\n" + "<br/>\n" + "<br/>\n" + "<br/>\n" + "<hr/>\n" + "<br/>\n" + "<br/>\n");
309381
}
310382
}
@@ -317,7 +389,7 @@ public StringBuilder generateReport(
317389
return stringBuilder;
318390
}
319391

320-
private static String getRepoUrl(String projectBaseDir) throws Exception {
392+
static String getRepoUrl(String projectBaseDir) throws Exception {
321393
String repoUrl;
322394
try (GitLogReader glr = new GitLogReader(new File(projectBaseDir))) {
323395
repoUrl = glr.getOriginUrl().replace(".git", "") + "/blob/" + glr.getCurrentCommitHash() + "/";
@@ -578,26 +650,13 @@ private String renderClassCycleSummary(List<RankedCycle> rankedCycles) {
578650
private String renderClassEdge(DefaultWeightedEdge edge) {
579651
StringBuilder edgesToCut = new StringBuilder();
580652
String[] vertexes = extractVertexes(edge);
581-
582653
String startVertex = vertexes[0].trim();
583-
String start;
584-
if (classesToRemove.contains(startVertex)) {
585-
start = "<strong>" + getClassName(startVertex) + "</strong>";
586-
} else {
587-
start = getClassName(startVertex);
588-
}
589-
590654
String endVertex = vertexes[1].trim();
591-
String end;
592-
if (classesToRemove.contains(endVertex)) {
593-
end = "<strong>" + getClassName(endVertex) + "</strong>";
594-
} else {
595-
end = getClassName(endVertex);
596-
}
597655

598656
// &#8594; is HTML "Right Arrow" code
599657
return edgesToCut
600-
.append(start + " &#8594; " + end + " : " + (int) classGraph.getEdgeWeight(edge))
658+
.append(getClassName(startVertex) + " &#8594; " + getClassName(endVertex) + " : "
659+
+ (int) classGraph.getEdgeWeight(edge))
601660
.toString();
602661
}
603662

@@ -683,8 +742,8 @@ private String renderSingleCycle(RankedCycle cycle, String repoUrl, CodebaseGrap
683742
stringBuilder.append("<br/>\n");
684743
stringBuilder.append("<br/>\n");
685744

686-
stringBuilder.append("<a id=\"CYCLEMAP\"><h2 align=\"center\">Largest Class Cycle : "
687-
+ getClassName(cycle.getCycleName()) + "</h2></a>\n");
745+
stringBuilder.append("<h2 align=\"center\"><a id=\"CYCLEMAP\">Largest Class Cycle : "
746+
+ getClassName(cycle.getCycleName()) + "</a></h2>\n");
688747
stringBuilder.append(
689748
"<h3 align=\"center\">Limiting number of cycles displayed to 1 to keep page load time fast</h3>\n");
690749
stringBuilder.append(renderClassCycleVisuals(cycle, repoUrl, codebaseGraphDTO));
@@ -728,7 +787,7 @@ private String renderSingleCycle(RankedCycle cycle, String repoUrl, CodebaseGrap
728787

729788
if (classRelationshipsToRemove.contains(edge)) {
730789
edges.append("<strong>");
731-
edges.append(renderClassEdge(edge));
790+
edges.append(renderClassEdge(edge) + "<strong>*</strong>");
732791
edges.append("</strong>");
733792
} else {
734793
edges.append(renderClassEdge(edge));
@@ -849,12 +908,12 @@ String getOutputName() {
849908
* Column headers are derived from the ranked metrics carried on each RankedDisharmony.
850909
*/
851910
public String renderDisharmonyInfo(
852-
String repoUrl,
853-
String anchorId,
854-
String title,
855-
boolean methodLevel,
856-
boolean showDetails,
857-
List<RankedDisharmony> ranked) {
911+
String repoUrl, DisharmonySpec spec, boolean showDetails, List<RankedDisharmony> ranked) {
912+
913+
String anchorId = spec.anchorId();
914+
String title = spec.title();
915+
boolean methodLevel = spec.methodLevel();
916+
858917
if (ranked.isEmpty()) {
859918
return "";
860919
}
@@ -868,6 +927,19 @@ public String renderDisharmonyInfo(
868927
.append(title)
869928
.append("</h1></a></div>\n");
870929

930+
sb.append("<div align=\"center\">");
931+
sb.append("<table border=\"5px\">\n");
932+
sb.append("<tr>\n");
933+
sb.append("<td><strong>Problem:</strong></td>");
934+
sb.append("<td>").append(spec.problem()).append("</td>\n");
935+
sb.append("</tr>\n");
936+
sb.append("<tr>\n");
937+
sb.append("<td><strong>Solution:</strong></td>");
938+
sb.append("<td>").append(spec.solution()).append("</td>\n");
939+
sb.append("</tr>\n");
940+
sb.append("</table>\n");
941+
sb.append("</div>\n");
942+
871943
sb.append(renderDisharmonyChart(anchorId, title, ranked, maxPriority));
872944

873945
sb.append("<h2 align=\"center\">")
@@ -1059,12 +1131,17 @@ static final class DisharmonySpec {
10591131
final String anchorId;
10601132
final String title;
10611133
final boolean methodLevel;
1134+
final String problem;
1135+
final String solution;
10621136

1063-
DisharmonySpec(String type, String anchorId, String title, boolean methodLevel) {
1137+
DisharmonySpec(
1138+
String type, String anchorId, String title, boolean methodLevel, String problem, String solution) {
10641139
this.type = type;
10651140
this.anchorId = anchorId;
10661141
this.title = title;
10671142
this.methodLevel = methodLevel;
1143+
this.problem = problem;
1144+
this.solution = solution;
10681145
}
10691146

10701147
String type() {
@@ -1082,5 +1159,13 @@ String title() {
10821159
boolean methodLevel() {
10831160
return methodLevel;
10841161
}
1162+
1163+
String problem() {
1164+
return problem;
1165+
}
1166+
1167+
String solution() {
1168+
return solution;
1169+
}
10851170
}
10861171
}

0 commit comments

Comments
 (0)