Core functionality to calculate metrics of single method #7

aravij · 2020-12-22T01:06:24Z

Adds test, picocli for cmd arguments parsing, PMD for checking format.

Two large files: gradlew and gradle.bat are autogenerated and need no review.

This reverts commit d92b41c

orchestr7 · 2020-12-24T12:12:12Z

build.gradle

+apply plugin: 'application'
+apply plugin: 'pmd'
+
+group 'com.github'


better to start with correct group - for example - you nick

Do you mean com.github.aravij?

orchestr7 · 2020-12-24T12:13:56Z

build.gradle

+apply plugin: 'pmd'
+
+group 'com.github'
+version '1.0-SNAPSHOT'


Better to use proper versioning. I strictly suggest https://semver.org/

orchestr7 · 2020-12-24T12:14:57Z

build.gradle

+}
+
+dependencies {
+    implementation group: 'net.sourceforge.pmd', name: 'pmd-java', version: '6.29.0'


it is fine to skip explicit "group"/"name"

example:

implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version"

Do you mean to change it to
implementation "net.sourceforge.pmd:pmd-java:6.29.0"

orchestr7 · 2020-12-24T12:15:49Z

build.gradle

+
+application {
+    mainClassName = 'com.github.emcat.Emcat'
+    applicationDefaultJvmArgs = ["--enable-preview"]


Better not to use preview features

In further PRs I get rid of preview mode and migrate to java 11.
We can keep it here and further PRs will fix this problem.

orchestr7 · 2020-12-24T12:20:41Z

use proper branch naming, like feature/my_feature

aravij · 2020-12-24T12:31:10Z

use proper branch naming, like feature/my_feature

In my word naming with number is a proper way. It allows to omit collision in names. And you can easily find it among issues, to read full text.

orchestr7 · 2020-12-24T14:09:49Z

use proper branch naming, like feature/my_feature

In my word naming with number is a proper way. It allows to omit collision in names. And you can easily find it among issues, to read full text.

this is not a best practice

orchestr7 · 2020-12-24T14:10:12Z

Also better to use MIT license

orchestr7 · 2020-12-24T14:10:55Z

src/main/java/com/github/emcat/Emcat.java

+
+        int exitCode;
+        if (methodMetrics == null) {
+            System.err.println("Failed to calculate metrics");


Java has a good logging mechanism, please use log4j, slf4j

orchestr7 · 2020-12-24T14:12:16Z

src/main/java/com/github/emcat/Emcat.java

+    private transient String className;
+
+    @Option(names = {"-m", "--method_name"}, required = true, description = "Method name to analyse")
+    private transient String methodName;


why transient here? no need for it, not related to serializaion

orchestr7 · 2020-12-24T14:12:33Z

src/main/java/com/github/emcat/Emcat.java

+@Command(name = "emcat", description = "Calculates given NCSS and cyclomatic complexity of a given method.", version = "1.0")
+public class Emcat implements Callable<Integer> {
+    @Option(names = {"-f", "--file"}, required = true, description = "File path to Java source code to analyse")
+    private transient String sourceCodeFilePath;


same about transient

orchestr7 · 2020-12-24T14:15:12Z

src/main/java/com/github/emcat/Emcat.java

+    }
+
+    public static void main(final String... args) {
+        System.exit(new CommandLine(new Emcat()).execute(args));//NOPMD


Why System.exit here? Better to exit in the place where you really want to exit.
System.exit should be used when the situation is extraordinary. Main should just simply return. It always return 0 by the default.

orchestr7 · 2020-12-24T14:16:10Z

src/main/java/com/github/emcat/Emcat.java

+import com.github.emcat.metric_calculators.MethodMetricsData;
+
+@Command(name = "emcat", description = "Calculates given NCSS and cyclomatic complexity of a given method.", version = "1.0")
+public class Emcat implements Callable<Integer> {


Why do you need callable interface here? No need for it, in your case you can just use a class.

orchestr7 · 2020-12-24T14:17:51Z

src/main/java/com/github/emcat/Emcat.java

+        if (methodMetrics == null) {
+            System.err.println("Failed to calculate metrics");
+            System.err.println(methodMetricsAggregator.getException());
+            exitCode = 1;


you can exit here

orchestr7 · 2020-12-24T14:20:56Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+
+            return metricsCalculator.calculateMethodMetrics(methodDeclaration);
+        }
+        catch (Exception exception) {


very bad practice to catch all exceptions here.

https://stackoverflow.com/questions/2416316/why-is-the-catchexception-almost-always-a-bad-idea

orchestr7 · 2020-12-24T14:22:42Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+        }
+    }
+
+    private Parser javaParser = new JavaLanguageModule().getDefaultVersion().getLanguageVersionHandler().getParser(new ParserOptions());


better to put properties in the beginning of the class

orchestr7 · 2020-12-24T14:40:36Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+
+    private ASTClassOrInterfaceDeclaration getClassDeclaration(String filename, String className) throws IOException {
+        ASTCompilationUnit compilationUnit = getCompilationUnit(filename);
+        for (JavaNode child : compilationUnit.children()) {


If you want to use great Java features - use lambda forEach( child -> )

orchestr7 · 2020-12-24T14:42:34Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+            }
+        }
+
+        throw new RuntimeException(String.format("There is no class named %s in file %s", className, filename));


Better not to build interaction logic on exceptions. Exit here with an error or return something good.

orchestr7 · 2020-12-24T14:42:49Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+
+    private ASTMethodDeclaration getMethodDeclaration(String filename, String className, String methodName) throws IOException {
+        ASTClassOrInterfaceDeclaration classDeclaration = getClassDeclaration(filename, className);
+        for (ASTAnyTypeBodyDeclaration classInnerDeclaration : classDeclaration.getDeclarations()) {


If you want to use great Java features - use lambda forEach( classInnerDeclaration -> )

orchestr7 · 2020-12-24T14:52:21Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+
+    private Parser javaParser = new JavaLanguageModule().getDefaultVersion().getLanguageVersionHandler().getParser(new ParserOptions());
+    private MethodMetricsCalculator metricsCalculator = new MethodMetricsCalculator();
+    private MethodProcessingException exception = null;


no need to initialize fields with null explicitly, in classes they are already initialized

orchestr7 · 2020-12-24T14:53:40Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+            }
+        }
+
+        throw new RuntimeException(String.format("There is no method named %s in class named %s in file %s", methodName, className, filename));


Same about exceptions as below, bad idea to build your logic using it. But if you really want to build your logic using that - at least create your own runtime exception, or choose suitable from existing

orchestr7 · 2020-12-24T14:59:50Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsCalculator.java

+
+    public MethodMetricsData calculateMethodMetrics(final ASTMethodOrConstructorDeclaration declarationNode) {
+        return new MethodMetricsData(
+            (int) ncssCalculator.computeFor(declarationNode, MetricOptions.emptyOptions()),


I suggest to be extremely careful here. Double is a bigger type so here it can cause a problem
May be it will be fine to check number ranges first

orchestr7 · 2020-12-24T15:00:25Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsCalculator.java

+import net.sourceforge.pmd.lang.metrics.MetricOptions;
+
+public class MethodMetricsCalculator {
+    private final transient CycloMetric cyclomaticComplexityCalculator = new CycloMetric();


why transient here? Not needed

orchestr7 · 2020-12-24T15:01:27Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsCalculator.java

+    private final transient CycloMetric cyclomaticComplexityCalculator = new CycloMetric();
+    private final transient NcssMetric.NcssOperationMetric ncssCalculator = new NcssMetric.NcssOperationMetric();
+
+    public MethodMetricsData calculateMethodMetrics(final ASTMethodOrConstructorDeclaration declarationNode) {


final here is useless

orchestr7 · 2020-12-24T15:03:26Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsCalculator.java

+    private final transient CycloMetric cyclomaticComplexityCalculator = new CycloMetric();
+    private final transient NcssMetric.NcssOperationMetric ncssCalculator = new NcssMetric.NcssOperationMetric();
+
+    public MethodMetricsData calculateMethodMetrics(final ASTMethodOrConstructorDeclaration declarationNode) {


In Java you can create constructor with arguments.
I suggest you to create a constructor here MethodMetricsCalculator (ASTMethodOrConstructorDeclaration declarationNode) with argument and do all the logic in it (for example to fill some MethodMetricsData field )

orchestr7 · 2020-12-24T15:16:26Z

src/main/java/com/github/emcat/metric_calculators/MethodProcessingException.java

+
+    private static final long serialVersionUID = -2272341134842435255L;
+
+    public MethodProcessingException(final MethodDescriptor method, final Exception exception) {


Wow, wrapping an exception is extremely bad practice

orchestr7

I suggest to read my comments, especially with logic related to exceptions

orchestr7 · 2020-12-24T16:11:49Z

src/main/java/com/github/emcat/metric_calculators/MethodMetricsAggregator.java

+package com.github.emcat.metric_calculators;
+
+import net.sourceforge.pmd.lang.Parser;
+import net.sourceforge.pmd.lang.ParserOptions;


all the code in this class is very heavy. I suggest you to create intermediate variables to make it less complex, for example instead of:

for (String a: foo.bar.test() ) {}

do:

String all = foo.bar.test(); all.forEach(a -> {} );

aravij added 6 commits December 21, 2020 15:51

Gradle build files and wrapper.

36207c1

Core functionality and test

1719bea

Add main class and CLI support.

7e4c126

Add PMD check.

a23db6e

Github CI.

d92b41c

Revert "Github CI."

d14e00b

This reverts commit d92b41c

aravij self-assigned this Dec 22, 2020

aravij linked an issue Dec 22, 2020 that may be closed by this pull request

Basic functionality #1

Open

aravij added the enhancement New feature or request label Dec 22, 2020

orchestr7 reviewed Dec 24, 2020

View reviewed changes

Fix build.gradle

7919d50

orchestr7 reviewed Dec 24, 2020

View reviewed changes

orchestr7 suggested changes Dec 24, 2020

View reviewed changes

orchestr7 reviewed Dec 24, 2020

View reviewed changes


		private static final long serialVersionUID = -2272341134842435255L;

		public MethodProcessingException(final MethodDescriptor method, final Exception exception) {

Core functionality to calculate metrics of single method #7

Are you sure you want to change the base?

Core functionality to calculate metrics of single method #7

Uh oh!

Conversation

aravij commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orchestr7 Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orchestr7 commented Dec 24, 2020

Uh oh!

aravij commented Dec 24, 2020

Uh oh!

orchestr7 commented Dec 24, 2020

Uh oh!

orchestr7 commented Dec 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orchestr7 Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orchestr7 Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orchestr7 Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orchestr7 Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aravij commented Dec 22, 2020 •

edited

Loading

orchestr7 Dec 24, 2020 •

edited

Loading

orchestr7 Dec 24, 2020 •

edited

Loading

orchestr7 Dec 24, 2020 •

edited

Loading

orchestr7 Dec 24, 2020 •

edited

Loading

orchestr7 Dec 24, 2020 •

edited

Loading

orchestr7 Dec 24, 2020 •

edited

Loading