Merge pull request #87 from refactorfirst/Refactor

Refactor codebase to improve analysis performance, HTML report output look, and developer ergonomics

Author: jimbethancourt

.github/workflows/maven.yml

@@ -24,8 +24,7 @@ jobs:
       - name: Build With Maven
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}  # Needed to get PR information, if any
-          SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
-        run: mvn -B verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar
+        run: mvn -B verify
 
 
 #      Comment "Build With Maven" and uncomment the below when you want a snapshot build to be deployed

change-proneness-ranker/src/main/java/org/hjug/git/ChangePronenessRanker.java

@@ -1,41 +1,40 @@
 package org.hjug.git;
 
 import java.io.IOException;
-import java.util.Comparator;
-import java.util.List;
-import java.util.TreeMap;
+import java.util.*;
 import lombok.extern.slf4j.Slf4j;
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.lib.Repository;
 
 @Slf4j
 public class ChangePronenessRanker {
 
-    private Repository repository;
-    private RepositoryLogReader repositoryLogReader;
-
-    public ChangePronenessRanker(Repository repository, RepositoryLogReader repositoryLogReader) {
-        this.repositoryLogReader = repositoryLogReader;
-        this.repository = repository;
-    }
-
-    public void rankChangeProneness(List<ScmLogInfo> scmLogInfos) {
-        TreeMap<Integer, Integer> changeCountsByTimeStamps = new TreeMap<>();
+    private final TreeMap<Integer, Integer> changeCountsByTimeStamps = new TreeMap<>();
+    private final Map<String, ScmLogInfo> cachedScmLogInfos = new HashMap<>();
 
+    public ChangePronenessRanker(Repository repository, GitLogReader repositoryLogReader) {
         try {
             log.info("Capturing change count based on commit timestamps");
             changeCountsByTimeStamps.putAll(repositoryLogReader.captureChangeCountByCommitTimestamp(repository));
         } catch (IOException | GitAPIException e) {
             log.error("Error reading from repository: {}", e.getMessage());
         }
+    }
 
+    public void rankChangeProneness(List<ScmLogInfo> scmLogInfos) {
         for (ScmLogInfo scmLogInfo : scmLogInfos) {
-            int commitsInRepositorySinceCreation =
-                    changeCountsByTimeStamps.tailMap(scmLogInfo.getEarliestCommit()).values().stream()
-                            .mapToInt(i -> i)
-                            .sum();
-
-            scmLogInfo.setChangeProneness((float) scmLogInfo.getCommitCount() / commitsInRepositorySinceCreation);
+            if (!cachedScmLogInfos.containsKey(scmLogInfo.getPath())) {
+                int commitsInRepositorySinceCreation =
+                        changeCountsByTimeStamps.tailMap(scmLogInfo.getEarliestCommit()).values().stream()
+                                .mapToInt(i -> i)
+                                .sum();
+
+                scmLogInfo.setChangeProneness((float) scmLogInfo.getCommitCount() / commitsInRepositorySinceCreation);
+                cachedScmLogInfos.put(scmLogInfo.getPath(), scmLogInfo);
+            } else {
+                scmLogInfo.setChangeProneness(
+                        cachedScmLogInfos.get(scmLogInfo.getPath()).getChangeProneness());
+            }
         }
 
         scmLogInfos.sort(Comparator.comparing(ScmLogInfo::getChangeProneness));

change-proneness-ranker/src/main/java/org/hjug/git/GitLogReader.java

@@ -15,15 +15,14 @@
 import org.eclipse.jgit.util.io.NullOutputStream;
 
 @Slf4j
-public class GitLogReader implements RepositoryLogReader {
+public class GitLogReader {
 
     static final String JAVA_FILE_TYPE = ".java";
 
     // Based on
     // https://github.com/Cosium/git-code-format-maven-plugin/blob/master/src/main/java/com/cosium/code/format/AbstractMavenGitCodeFormatMojo.java
     // MIT License
     // Move to a provider?
-    @Override
     public Repository gitRepository(File basedir) throws IOException {
         Repository gitRepository;
         FileRepositoryBuilder repositoryBuilder = new FileRepositoryBuilder().findGitDir(basedir);
@@ -46,7 +45,6 @@ public File getGitDir(File basedir) {
     // https://stackoverflow.com/a/19950970/346247
     // and
     // https://github.com/centic9/jgit-cookbook/blob/master/src/main/java/org/dstadler/jgit/api/ReadFileFromCommit.java
-    @Override
     public Map<String, ByteArrayOutputStream> listRepositoryContentsAtHEAD(Repository repository) throws IOException {
         Ref head = repository.exactRef("HEAD");
         // a RevWalk allows us to walk over commits based on some filtering that is defined
@@ -88,7 +86,6 @@ public Map<String, ByteArrayOutputStream> listRepositoryContentsAtHEAD(Repositor
      * @return a LogInfo object
      * @throws GitAPIException
      */
-    @Override
     public ScmLogInfo fileLog(Repository repository, String path) throws GitAPIException, IOException {
         Git git = new Git(repository);
         ObjectId branchId = repository.resolve("HEAD");
@@ -118,7 +115,6 @@ public ScmLogInfo fileLog(Repository repository, String path) throws GitAPIExcep
     }
 
     // based on https://stackoverflow.com/questions/27361538/how-to-show-changes-between-commits-with-jgit
-    @Override
     public TreeMap<Integer, Integer> captureChangeCountByCommitTimestamp(Repository repository)
             throws IOException, GitAPIException {
 

change-proneness-ranker/src/main/java/org/hjug/git/RepositoryLogReader.java

@@ -11,15 +11,14 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
-public class ChangePronenessRankerTest {
+class ChangePronenessRankerTest {
 
     private ChangePronenessRanker changePronenessRanker;
-    private RepositoryLogReader repositoryLogReader;
+    private GitLogReader repositoryLogReader;
 
     @BeforeEach
     public void setUp() {
-        repositoryLogReader = mock(RepositoryLogReader.class);
-        changePronenessRanker = new ChangePronenessRanker(null, repositoryLogReader);
+        repositoryLogReader = mock(GitLogReader.class);
     }
 
     // TODO: this should probably be a cucumber test
@@ -34,6 +33,7 @@ void testChangePronenessCalculation() throws IOException, GitAPIException {
 
         when(repositoryLogReader.captureChangeCountByCommitTimestamp(any())).thenReturn(commitsWithChangeCounts);
 
+        changePronenessRanker = new ChangePronenessRanker(null, repositoryLogReader);
         List<ScmLogInfo> scmLogInfos = new ArrayList<>();
         scmLogInfos.add(scmLogInfo);
         changePronenessRanker.rankChangeProneness(scmLogInfos);
@@ -58,6 +58,7 @@ void testRankChangeProneness() throws IOException, GitAPIException {
         commitsWithChangeCounts.put(scmLogInfo2.getEarliestCommit() + 10 * 60, 5);
 
         when(repositoryLogReader.captureChangeCountByCommitTimestamp(any())).thenReturn(commitsWithChangeCounts);
+        changePronenessRanker = new ChangePronenessRanker(null, repositoryLogReader);
 
         List<ScmLogInfo> scmLogInfos = new ArrayList<>();
         scmLogInfos.add(scmLogInfo);

change-proneness-ranker/src/test/java/org/hjug/git/ChangePronenessRankerTest.java

@@ -60,6 +60,16 @@
                                     <mainClass>org.hjug.refactorfirst.Main</mainClass>
                                 </transformer>
                             </transformers>
+                            <filters>
+                                <filter>
+                                    <artifact>*:*</artifact>
+                                    <excludes>
+                                        <exclude>META-INF/*.SF</exclude>
+                                        <exclude>META-INF/*.DSA</exclude>
+                                        <exclude>META-INF/*.RSA</exclude>
+                                    </excludes>
+                                </filter>
+                            </filters>
                         </configuration>
                     </execution>
                 </executions>

cli/pom.xml

@@ -11,6 +11,7 @@
 import org.apache.maven.project.MavenProject;
 import org.hjug.refactorfirst.report.CsvReport;
 import org.hjug.refactorfirst.report.HtmlReport;
+import org.hjug.refactorfirst.report.SimpleHtmlReport;
 import org.hjug.refactorfirst.report.json.JsonReportExecutor;
 import picocli.CommandLine.Command;
 
@@ -58,6 +59,10 @@ public Integer call() {
         inferArgumentsFromMavenProject();
         populateDefaultArguments();
         switch (reportType) {
+            case SIMPLE_HTML:
+                SimpleHtmlReport simpleHtmlReport = new SimpleHtmlReport();
+                simpleHtmlReport.execute(showDetails, projectName, projectVersion, outputDirectory, baseDir);
+                return 0;
             case HTML:
                 HtmlReport htmlReport = new HtmlReport();
                 htmlReport.execute(showDetails, projectName, projectVersion, outputDirectory, baseDir);

cli/src/main/java/org/hjug/refactorfirst/ReportCommand.java

@@ -1,6 +1,7 @@
 package org.hjug.refactorfirst;
 
 public enum ReportType {
+    SIMPLE_HTML,
     HTML,
     JSON,
     CSV;

cli/src/main/java/org/hjug/refactorfirst/ReportType.java

@@ -18,7 +18,6 @@
 import org.eclipse.jgit.lib.Repository;
 import org.hjug.git.ChangePronenessRanker;
 import org.hjug.git.GitLogReader;
-import org.hjug.git.RepositoryLogReader;
 import org.hjug.git.ScmLogInfo;
 import org.hjug.metrics.*;
 import org.hjug.metrics.rules.CBORule;
@@ -27,11 +26,30 @@
 public class CostBenefitCalculator {
 
     private Report report;
-    private String projBaseDir = null;
+    private String repositoryPath;
+    private final GitLogReader gitLogReader = new GitLogReader();
+    private Repository repository = null;
+    private final ChangePronenessRanker changePronenessRanker;
+
+    public CostBenefitCalculator(String repositoryPath) {
+        this.repositoryPath = repositoryPath;
+
+        log.info("Initiating Cost Benefit calculation");
+        try {
+            repository = gitLogReader.gitRepository(new File(repositoryPath));
+            for (String file :
+                    gitLogReader.listRepositoryContentsAtHEAD(repository).keySet()) {
+                log.info("Files at HEAD: {}", file);
+            }
+        } catch (IOException e) {
+            log.error("Failure to access Git repository", e);
+        }
+
+        changePronenessRanker = new ChangePronenessRanker(repository, gitLogReader);
+    }
 
     // copied from PMD's PmdTaskImpl.java and modified
-    public void runPmdAnalysis(String projectBaseDir) throws IOException {
-        projBaseDir = projectBaseDir;
+    public void runPmdAnalysis() throws IOException {
         PMDConfiguration configuration = new PMDConfiguration();
 
         try (PmdAnalysis pmd = PmdAnalysis.create(configuration)) {
@@ -42,35 +60,20 @@ public void runPmdAnalysis(String projectBaseDir) throws IOException {
             cboClassRule.setLanguage(LanguageRegistry.PMD.getLanguageByFullName("Java"));
             pmd.addRuleSet(RuleSet.forSingleRule(cboClassRule));
 
-            log.info("files to be scanned: " + Paths.get(projectBaseDir));
+            log.info("files to be scanned: " + Paths.get(repositoryPath));
 
-            try (Stream<Path> files = Files.walk(Paths.get(projectBaseDir))) {
+            try (Stream<Path> files = Files.walk(Paths.get(repositoryPath))) {
                 files.forEach(file -> pmd.files().addFile(file));
             }
 
             report = pmd.performAnalysisAndCollectReport();
         }
     }
 
-    public List<RankedDisharmony> calculateGodClassCostBenefitValues(String repositoryPath) {
-
-        RepositoryLogReader repositoryLogReader = new GitLogReader();
-        Repository repository = null;
-        log.info("Initiating Cost Benefit calculation");
-        try {
-            repository = repositoryLogReader.gitRepository(new File(repositoryPath));
-            for (String file :
-                    repositoryLogReader.listRepositoryContentsAtHEAD(repository).keySet()) {
-                log.info("Files at HEAD: {}", file);
-            }
-        } catch (IOException e) {
-            log.error("Failure to access Git repository", e);
-        }
-
-        // pass repo path here, not ByteArrayOutputStream
+    public List<RankedDisharmony> calculateGodClassCostBenefitValues() {
         List<GodClass> godClasses = getGodClasses();
 
-        List<ScmLogInfo> scmLogInfos = getRankedChangeProneness(repositoryLogReader, repository, godClasses);
+        List<ScmLogInfo> scmLogInfos = getRankedChangeProneness(godClasses);
 
         Map<String, ScmLogInfo> rankedLogInfosByPath =
                 scmLogInfos.stream().collect(Collectors.toMap(ScmLogInfo::getPath, logInfo -> logInfo, (a, b) -> b));
@@ -114,15 +117,14 @@ private List<GodClass> getGodClasses() {
         return godClasses;
     }
 
-    <T extends Disharmony> List<ScmLogInfo> getRankedChangeProneness(
-            RepositoryLogReader repositoryLogReader, Repository repository, List<T> disharmonies) {
+    <T extends Disharmony> List<ScmLogInfo> getRankedChangeProneness(List<T> disharmonies) {
         List<ScmLogInfo> scmLogInfos = new ArrayList<>();
         log.info("Calculating Change Proneness");
         for (Disharmony disharmony : disharmonies) {
             String path = disharmony.getFileName();
             ScmLogInfo scmLogInfo = null;
             try {
-                scmLogInfo = repositoryLogReader.fileLog(repository, path);
+                scmLogInfo = gitLogReader.fileLog(repository, path);
                 log.info("Successfully fetched scmLogInfo for {}", scmLogInfo.getPath());
             } catch (GitAPIException | IOException e) {
                 log.error("Error reading Git repository contents.", e);
@@ -136,25 +138,14 @@ <T extends Disharmony> List<ScmLogInfo> getRankedChangeProneness(
             }
         }
 
-        ChangePronenessRanker changePronenessRanker = new ChangePronenessRanker(repository, repositoryLogReader);
         changePronenessRanker.rankChangeProneness(scmLogInfos);
         return scmLogInfos;
     }
 
-    public List<RankedDisharmony> calculateCBOCostBenefitValues(String repositoryPath) {
-
-        RepositoryLogReader repositoryLogReader = new GitLogReader();
-        Repository repository = null;
-        log.info("Initiating Cost Benefit calculation");
-        try {
-            repository = repositoryLogReader.gitRepository(new File(repositoryPath));
-        } catch (IOException e) {
-            log.error("Failure to access Git repository", e);
-        }
-
+    public List<RankedDisharmony> calculateCBOCostBenefitValues() {
         List<CBOClass> cboClasses = getCBOClasses();
 
-        List<ScmLogInfo> scmLogInfos = getRankedChangeProneness(repositoryLogReader, repository, cboClasses);
+        List<ScmLogInfo> scmLogInfos = getRankedChangeProneness(cboClasses);
 
         Map<String, ScmLogInfo> rankedLogInfosByPath =
                 scmLogInfos.stream().collect(Collectors.toMap(ScmLogInfo::getPath, logInfo -> logInfo, (a, b) -> b));
@@ -193,6 +184,6 @@ private List<CBOClass> getCBOClasses() {
     }
 
     private String getFileName(RuleViolation violation) {
-        return violation.getFileId().getUriString().replace("file:///" + projBaseDir.replace("\\", "/") + "/", "");
+        return violation.getFileId().getUriString().replace("file:///" + repositoryPath.replace("\\", "/") + "/", "");
     }
 }

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

@@ -47,10 +47,10 @@ void testCBOViolation() throws IOException, GitAPIException, InterruptedExceptio
         git.add().addFilepattern(".").call();
         RevCommit firstCommit = git.commit().setMessage("message").call();
 
-        CostBenefitCalculator costBenefitCalculator = new CostBenefitCalculator();
-        costBenefitCalculator.runPmdAnalysis(git.getRepository().getDirectory().getParent());
-        List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateCBOCostBenefitValues(
-                git.getRepository().getDirectory().getPath());
+        CostBenefitCalculator costBenefitCalculator =
+                new CostBenefitCalculator(git.getRepository().getDirectory().getParent());
+        costBenefitCalculator.runPmdAnalysis();
+        List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateCBOCostBenefitValues();
 
         Assertions.assertFalse(disharmonies.isEmpty());
     }
@@ -80,10 +80,10 @@ void testCostBenefitCalculation() throws IOException, GitAPIException, Interrupt
         git.add().addFilepattern(".").call();
         RevCommit secondCommit = git.commit().setMessage("message").call();
 
-        CostBenefitCalculator costBenefitCalculator = new CostBenefitCalculator();
-        costBenefitCalculator.runPmdAnalysis(git.getRepository().getDirectory().getParent());
-        List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateGodClassCostBenefitValues(
-                git.getRepository().getDirectory().getPath());
+        CostBenefitCalculator costBenefitCalculator =
+                new CostBenefitCalculator(git.getRepository().getDirectory().getParent());
+        costBenefitCalculator.runPmdAnalysis();
+        List<RankedDisharmony> disharmonies = costBenefitCalculator.calculateGodClassCostBenefitValues();
 
         Assertions.assertEquals(1, disharmonies.get(0).getRawPriority().intValue());
         Assertions.assertEquals(1, disharmonies.get(1).getRawPriority().intValue());