[java] Groundwork for the upcoming metrics framework #409

oowekyala · 2017-05-26T14:42:26Z

So we figured with @adangel that I should not wait too much to make a PR for my work on the metrics framework, given the increasing mess that is the history of this branch. In the future I'll do smaller PRs

Note: The framework is absolutely not functional, but should not break anything

Changelog

Changes to the AST

Addition of a marker interface that groups ASTConstructorDeclaration and ASTMethodDeclaration together
Addition of a QualifiableNode interface:
- Implemented by FieldDeclaration, MethodDeclaration, ConstructorDeclaration, ClassOrInterfaceDeclaration
- Adds method QualifiedName getQualifiedName(), which retrieves the qualified name of the node
- QualifiedName's implementation is nested inside QualifiableNode
- I added unit tests for Qualifiedname but they will be perfected when I'm sure which specification I need

New stuff

New package oom (object oriented metrics) in lang.java, to contain the framework
Class oom.Metrics : Façade of the framework
Package oom.metrics : will contain the implementations of the metrics (there are already some drafts in there)
Package oom.visitor : contains the visitor that will be executed before the rules and related classes. About the visitor:
- It only triggers if at least one rule of the ruleset has the metrics attribute set to true
- It fills a data structure implemented by PackageStats, which stores information about the methods and fields of each class. We use QualifiedName to address the contents of the DS and make requests
- The information stored about methods and fields is represented by Signature classes. More info on signatures in my fork's wiki

…to metrics-prework

…s-prework

…ow because of Typeres. Be sure to edit the tests

adangel

Awesome, great work! I really like seeing the metrics framework evolving!

adangel · 2017-05-29T19:35:27Z

pmd-core/src/main/java/net/sourceforge/pmd/RuleSets.java

@@ -201,6 +201,16 @@ public boolean usesTypeResolution(Language language) {
        return false;
    }

+    public boolean usesMetrics(Language language) {


Please add for this directly the javadocs. I know, "usesTyperesolution" above doesn't have the docs either, but somewhere we need to start :)

Right I'll document both then ;)

adangel · 2017-05-29T19:38:50Z

pmd-core/src/main/java/net/sourceforge/pmd/benchmark/Benchmark.java

-    MeasuredTotal(12, "Measured total"),
-    NonMeasuredTotal(13, "Non-measured total"),
-    TotalPMD(14, "Total PMD");
+    MetricsVisitor(8, "Metrics visitor"),


Just a minor thing: I think, I wouldn't name this here MetricsVisitor - maybe just Metrics? Since "Visitor" is just an implementation detail...

adangel · 2017-05-29T19:43:06Z

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java

@@ -567,6 +567,10 @@ private void parseSingleRuleNode(RuleSetReferenceId ruleSetReferenceId, RuleSetB
            rule.setUsesTypeResolution();
        }

+        if (hasAttributeSetTrue(ruleElement, "metrics")) {


I'll need to lookup myself, but I guess, we need to update somehow the XML schema / DTD. Otherwise every XML editor will complain, that the attribute "metrics" is not allowed and one cannot create a ruleset xml file, with rules, that use metrics ...

Good point! Is this repo the sources for http://pmd.sourceforge.net/ ? In that case I think adding a line to this xsd definition, which is loaded by rulesets, would do the trick

Yes, that's one place, were the definition can be downloaded. But the master source is really in the main repo (under pmd-core): https://github.com/pmd/pmd/tree/master/pmd-core/src/main/resources

I'm not sure about whether we could just change the definition and keep the version/namespace the same (given that it is compatible) or whether we should change the namespace...

adangel · 2017-05-29T19:43:56Z

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java

@@ -567,6 +567,10 @@ private void parseSingleRuleNode(RuleSetReferenceId ruleSetReferenceId, RuleSetB
            rule.setUsesTypeResolution();
        }

+        if (hasAttributeSetTrue(ruleElement, "metrics")) {


And of course, we shouldn't forget about documenting this... but that's something for later.

adangel · 2017-05-29T19:47:53Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceDeclaration.java

+        if (isNested()) {
+            ASTClassOrInterfaceDeclaration parent = this.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class);
+            QualifiedName parentQN = parent.getQualifiedName();
+            return QualifiedName.makeClassOf(parentQN, this.getImage());


Should we cache the qualifiedName in this.qualifiedName here, too? For the other case (not isNested()), we cache it. Since the AST doesn't change, I think, it's safe to cache it here the same way.

That's actually a nice recursive call 👍 Goes up the tree to the outer class. I hope, isNested() returns false, then...

Thanks, I forgot to cache it there! Well yes otherwise we'd get a nice NPE :)

adangel · 2017-05-29T20:23:07Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/Metrics.java

+     */
+    public enum OperationMetricKey {
+
+        ATFD(new AtfdMetric());


Just wondering: is AFTD both a ClassMetric and a OperationMetric, or is this just an example?

Yes, ATFD is both

adangel · 2017-05-29T20:26:12Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/metrics/AtfdMetric.java

+            }
+        }
+
+        return foreignCalls / callQNames.size();


This look like an integer division. You'll need to cast one number to a double before dividing....

adangel · 2017-05-29T20:28:29Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/metrics/LocMetric.java

+
+    @Override
+    public double computeFor(ASTClassOrInterfaceDeclaration node, PackageStats holder) {
+        return node.getEndLine() - node.getBeginLine();


This is a very simple method to calculate the lines. It depends on the source code formatting. I don't know about the requirements for this particular metric, but maybe NCSS (non-commenting sourcecode statements) might be an alternative. It of course it a bit more complex to calculate...

I didn't put much thought into it lol

I guess we could have both metrics, one version that counts with comments and the other not, and set which implementation will be used by rules that use LOC via a rule property.

adangel · 2017-05-29T20:31:23Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/visitor/OperationSignature.java

+    public enum Role {
+        GETTER_OR_SETTER, CONSTRUCTOR, METHOD, STATIC;
+
+        public static final Role[] ALL = {GETTER_OR_SETTER, CONSTRUCTOR, METHOD, STATIC};


The enum provides already a field values which contains all the values of the enum in an array. So, no need to define ALL manually :)

adangel · 2017-05-29T20:32:12Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/oom/visitor/Signature.java

+    public enum Visibility {
+        PUBLIC, PACKAGE, PROTECTED, PRIVATE, UNDEF;
+
+        public static final Visibility[] ALL = {PUBLIC, PACKAGE, PROTECTED, PRIVATE};


Same here: you can use Visibility.values instead

The creation methods now mostly take the node to describe as parameter. That way, the implementation of the class is less exposed, the creation methods can be overloaded and simplified in the AST nodes' classes. Only nested class do not do that. That is to keep the recursive call to getQualifiedName which would be much more complicated if it was called from QualifiedName rather than the AST node.

oowekyala and others added 30 commits March 26, 2017 23:51

Simple Key enum selection

e26b715

Double enum selection

08b6b56

Started holder impl

3a942aa

Started holder impl

f614554

Implemented signatures

7544aa4

Abstracted signatures

e569941

Support for constructors

8ff272a

Simple Key enum selection

9fedb69

Double enum selection

af650cc

Started holder impl

b922bcd

Started holder impl

92568e3

Implemented signatures

7a5894c

Abstracted signatures

4b671f5

Support for constructors

1ad7487

Merge branch 'metrics-prework' of https://github.com/Oowekyala/pmd in…

6e6559c

…to metrics-prework

Merge branch 'master' into metrics-prework

ebafeff

Merge branch 'master' into metrics-prework

5d9434b

Introduced marker interface for AST method and constructor nodes

5d308a5

Simplified capability check

6a0cd33

Methods and constructors are treated the same

8af2b6c

Changed header template

a8ff116

Documentation

cb0a30b

Changed package structure

9c1e8bb

Cleanup

16a878b

References to class hierarchy

3c36481

Outer memo infrastructure

0193aa8

Merge branch 'master' of https://github.com/oowekyala/pmd into metric…

9ec2388

…s-prework

Added OO metrics to Rule interface

cdd8737

Implemented usesMetrics field in Rules

8bc1173

Added usesMetrics to Rule interface

c0b020e

oowekyala added 12 commits May 26, 2017 14:23

Fix benchmark

117da90

Use Qname for memoized result retrieval

1673e83

Inlined canonical separators for qualified names

909bb60

Canonical type of arguments in method Qname\n That doesn't work for n…

b09cde6

…ow because of Typeres. Be sure to edit the tests

Revert "Canonical type of arguments in method Qname" until TypeRes is ok

5b918ba

Fix capabilities nonsense

0847f24

Deleted local file

acff55e

Revert dumb move with gitignore

ad51c61

Checkstyle fixes

6fbcb43

Checkstyle fix

e1f02ac

Fixed formatter chaos

4be7616

Fix misplaced benchmark

da4897f

adangel self-assigned this May 28, 2017

adangel self-requested a review May 28, 2017 17:20

adangel requested changes May 29, 2017

View reviewed changes

oowekyala added 7 commits May 30, 2017 13:34

Moved QualifiedName into standalone class

02bac9a

Documented usesMetrics and usesTypeRes

9ad46fb

Remove ALL fields in enums

9ee19ac

Checkstyle fix

0aff989

Changed benchmark name

255dfd0

Merge branch 'master' of https://github.com/pmd/pmd into metrics-prework

455e42b

adangel added an:enhancement An improvement on existing features / rules in:pmd-internals Affects PMD's internals labels May 31, 2017

adangel approved these changes May 31, 2017

View reviewed changes

adangel added the in:metrics Affects the metrics framework label Jun 2, 2017

adangel added a commit to adangel/pmd that referenced this pull request Jun 2, 2017

Update changelog, refs pmd#409

5c8ac24

adangel merged commit 455e42b into pmd:master Jun 2, 2017

adangel added this to the 5.8.0 milestone Jun 2, 2017

oowekyala deleted the metrics-prework branch July 10, 2017 19:12

Uh oh!

[java] Groundwork for the upcoming metrics framework #409

[java] Groundwork for the upcoming metrics framework #409

Uh oh!

Conversation

oowekyala commented May 26, 2017

Changelog

Changes to the AST

New stuff

Uh oh!

adangel left a comment

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!

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!

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!

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!

Uh oh!