diff --git a/org.fortiss.tooling.ext.quality/html/developer/.ratings b/org.fortiss.tooling.ext.quality/html/developer/.ratings
index 615211454f1b58f7ed50bd8eef1ee53734bc18fb..b1f9061030d5d87f34d41998eba0091beef2e7af 100644
--- a/org.fortiss.tooling.ext.quality/html/developer/.ratings
+++ b/org.fortiss.tooling.ext.quality/html/developer/.ratings
@@ -1 +1 @@
-documentation.html a9df919adc2e879d79775a358a858f77ebbbaf86 YELLOW
+documentation.html f102e6b3757eec158de9bf78238e2865d01de2a4 RED
diff --git a/org.fortiss.tooling.ext.quality/html/developer/documentation.html b/org.fortiss.tooling.ext.quality/html/developer/documentation.html
index a9df919adc2e879d79775a358a858f77ebbbaf86..f102e6b3757eec158de9bf78238e2865d01de2a4 100644
--- a/org.fortiss.tooling.ext.quality/html/developer/documentation.html
+++ b/org.fortiss.tooling.ext.quality/html/developer/documentation.html
@@ -139,7 +139,7 @@
<tr>
<td>15</td>
<td>ComponentMetricProvider</td>
- <td>org.fortiss.af3.component.quality.ComponentMetricProvider</td>
+ <td>TODO (SB): Documentation of providers should be distributed to respective plugins. org.fortiss.af3.component.quality.ComponentMetricProvider</td>
<td>NUMBER_OF_TOTAL_CODE_CONSTANTS</td>
<td>Integer</td>
<td>Total number of constants in code specifications in this element and all contained elements.</td>
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/.ratings b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/.ratings
index 8f7af8e81f7fc36381e2a67be8031224c539c368..a0dfdcdd32991599396186c0c2bc9523ef06bc69 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/.ratings
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/.ratings
@@ -1,5 +1,5 @@
-AF3QualityActivator.java 353c3d99f423997e4e99a896b3c095fd77d81431 YELLOW
-GraphMetricsProvider.java 021a9a7de4fb080fe3a7f4f1fe1843564080cdb4 YELLOW
-HierarchicElementProvider.java 14d99b4d76ec28191ecd0f2654f43b92c36f1302 YELLOW
-IMetricProvider.java 99fc8993b0e65b2f8757978eeb0481d912f5608c YELLOW
-IMetricUpdateListener.java c24dc7c0f282623bbf1eefac1fbbb6752c97ddf0 YELLOW
+AF3QualityActivator.java 9f2f29b8d55c033e1df16320b0b8a1b6f0446f60 RED
+GraphMetricsProvider.java 0d760fe19417a080016ebc4bfcc4c2753bde1e59 RED
+HierarchicElementProvider.java cbe405204b08931fd5d219570d0ffb2ab49f27b7 RED
+IMetricProvider.java 99fc8993b0e65b2f8757978eeb0481d912f5608c GREEN
+IMetricUpdateListener.java c24dc7c0f282623bbf1eefac1fbbb6752c97ddf0 GREEN
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/AF3QualityActivator.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/AF3QualityActivator.java
index 353c3d99f423997e4e99a896b3c095fd77d81431..9f2f29b8d55c033e1df16320b0b8a1b6f0446f60 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/AF3QualityActivator.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/AF3QualityActivator.java
@@ -22,6 +22,7 @@ import org.fortiss.tooling.ext.quality.service.IModelQualityService;
import org.fortiss.tooling.ext.quality.service.ModelQualityService;
import org.osgi.framework.BundleContext;
+// TODO (SB): Rename class to QualityActivator or ModelQualityActivator. This is the kernel, not AF3
/**
* The activator class controls the plug-in life cycle.
*
@@ -41,6 +42,7 @@ public class AF3QualityActivator extends Plugin {
plugin = this;
System.out.println("[Plugin] " + PLUGIN_ID + " started.");
+ // TODO: Use only IModelQualityService. Store it in a variable with a brief name, e.g., mqs
ModelQualityService.getInstance().startService();
IModelQualityService.getInstance().registerMetricProvider(new HierarchicElementProvider(),
IHierarchicElement.class);
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/GraphMetricsProvider.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/GraphMetricsProvider.java
index 021a9a7de4fb080fe3a7f4f1fe1843564080cdb4..0d760fe19417a080016ebc4bfcc4c2753bde1e59 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/GraphMetricsProvider.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/GraphMetricsProvider.java
@@ -40,6 +40,10 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
*
* @author groh
*/
+
+// TODO (SB): shouldn't this implement the interface IMetricProvider? If it is not possible to
+// factorize this now, but the suggestions makes sense, create a ticket and reference it here
+// using TODO (#xyz): blah). Otherwise, consider renaming the class to avoid the confusion.
public class GraphMetricsProvider {
/**
@@ -63,6 +67,8 @@ public class GraphMetricsProvider {
} else {
graphNodes.addAll(scopeElement.getContainedElements());
}
+
+ // TODO (SB): Static import
// Collect all elements to which currentElement has a outgoing connection
for(ExitConnectorBase exitConnector : EcoreUtils.pickInstanceOf(ExitConnectorBase.class,
scopeElement.getConnectors())) {
@@ -70,6 +76,7 @@ public class GraphMetricsProvider {
graphNodes.add(exitConnection.getTarget().getOwner());
}
}
+ // TODO (SB): Static import
// Collect all elements to which currentElement has a incoming connection
for(EntryConnectorBase entryConnector : EcoreUtils.pickInstanceOf(EntryConnectorBase.class,
scopeElement.getConnectors())) {
@@ -154,10 +161,12 @@ public class GraphMetricsProvider {
private static Set<IHierarchicElement> getUndirectedNeighbors(IHierarchicElement element) {
Set<IHierarchicElement> neighbors = new HashSet<>();
+ // TODO (SB): Static import
for(ExitConnectorBase exitConnectors : EcoreUtils.pickInstanceOf(ExitConnectorBase.class,
element.getConnectors())) {
recursivlyFollowOutgoingConnection(exitConnectors, neighbors);
}
+ // TODO (SB): Static import
for(EntryConnectorBase entryConnectors : EcoreUtils.pickInstanceOf(EntryConnectorBase.class,
element.getConnectors())) {
recursivlyFollowIncomingConnection(entryConnectors, neighbors);
@@ -165,6 +174,8 @@ public class GraphMetricsProvider {
return neighbors;
}
+ // TODO (SB): Pleae provide a link (DOI to paper, Wikipedia, ...) to a definition of the
+ // clustering coefficient? The algorithm is not obvious...
/**
* Calculates the clustering coefficient based on the neighborhood.
*
@@ -198,6 +209,8 @@ public class GraphMetricsProvider {
return clusteringCoefficient;
}
+ // TODO (SB): Please provide a link (DOI to paper, Wikipedia, ...) to a definition of the
+ // betweenness centrality. The algorithm is not obvious...
/**
* Calculates and saves the betweenness centrality of all elements contained inside the provided
* scopeElement.
@@ -208,10 +221,13 @@ public class GraphMetricsProvider {
* location to save the metrics
* @param recursively
* if true, all calculation will be carried out on leaf elements
+ * TODO (SB):, otherwise.... <not obvious :-) >
*/
public static void calculateBetweennessCentrality(IHierarchicElement scopeElement,
MetricDataManager manager, boolean recursively) {
+ // TODO (SB): Use inverse logic and return to decrease nesting depth
+ // if (foo) { return; } bar();
// Abort if no elements are found
if(!scopeElement.getContainedElements().isEmpty()) {
@@ -305,6 +321,7 @@ public class GraphMetricsProvider {
Map<IHierarchicElement, Integer> distance, Map<IHierarchicElement, Double> dependency,
Set<IHierarchicElement> graphNodes, IHierarchicElement scopeElement,
boolean recursively) {
+
while(!queue.isEmpty()) {
IHierarchicElement v = queue.remove();
stack.push(v);
@@ -342,10 +359,15 @@ public class GraphMetricsProvider {
// Check if element is in node list, to avoid creating data for
// nodes outside of our graph
if(graphNodes.contains(targetElement)) {
+ // TODO (SB): Introduce local variables with good names for
+ // distance.get(targetElement) and distance.get(v)
if(distance.get(targetElement) < 0) {
queue.add(targetElement);
distance.put(targetElement, distance.get(v) + 1);
}
+ // TODO (SB): Just for safety-checking: Is it intended, there there is
+ // no "else", i.e. that both conditions could be true and both branches
+ // should be entered?
if(distance.get(targetElement) == distance.get(v) + 1) {
dependency.put(targetElement,
dependency.get(targetElement) + dependency.get(v));
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/HierarchicElementProvider.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/HierarchicElementProvider.java
index 14d99b4d76ec28191ecd0f2654f43b92c36f1302..cbe405204b08931fd5d219570d0ffb2ab49f27b7 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/HierarchicElementProvider.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/HierarchicElementProvider.java
@@ -29,11 +29,13 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
*
* @author groh
*/
+// TODO (SB): Rename to HierarchicElementMetricProvider
public class HierarchicElementProvider implements IMetricProvider<IHierarchicElement> {
/** {@inheritDoc} */
@Override
public void collectMetrics(MetricTreeNode node, IHierarchicElement currentElement) {
+ // TODO (SB): Use better variable name, e.g. intMetrics. See also comment in MetricTreeNode
var integers = node.getStoredIntegers();
// Only collect these metrics if the name is null, in which case they have not been
// collected before
@@ -41,6 +43,10 @@ public class HierarchicElementProvider implements IMetricProvider<IHierarchicEle
if(node.getName() == null) {
// Metrics are always set to default value, as these metric are combined later
+ // TODO (SB): The ternary operators here very hard to read, and later on you use an if
+ // anyways.
+ // Suggestion: if (currentElement instanceof INamedCommentedElement) { bla} else {
+ // blub}.
boolean isElementCommentable = currentElement instanceof INamedCommentedElement;
integers.put(MetricKey.NUMBER_OF_TOTAL_COMMENTABLE_ELEMENTS,
isElementCommentable ? 1 : 0);
@@ -67,6 +73,7 @@ public class HierarchicElementProvider implements IMetricProvider<IHierarchicEle
// depth metrics
integers.put(MetricKey.NUMBER_OF_TOTAL_CONNECTORS, connectors.size());
+ // TODO (SB): Static import (CTRL+SHIFT+M). (2x)
var entry_connectors = EcoreUtils.pickInstanceOf(EntryConnectorBase.class, connectors);
var exit_connectors = EcoreUtils.pickInstanceOf(ExitConnectorBase.class, connectors);
integers.put(MetricKey.NUMBER_OF_TOTAL_ENTRY_CONNECTORS, entry_connectors.size());
@@ -74,6 +81,7 @@ public class HierarchicElementProvider implements IMetricProvider<IHierarchicEle
integers.put(MetricKey.NUMBER_OF_TOTAL_ELEMENTS, 1);
+ // TODO (SB): size() == 0 --> isEmpty()
integers.put(MetricKey.NUMBER_OF_TOTAL_LEAF_ELEMENTS,
currentElement.getContainedElements().size() == 0 ? 1 : 0);
}
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/.ratings b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/.ratings
index 39c11d23b8ae826708c09a559e900e082c6633cb..fbdbd5aef728a4bbee5b090d775833b6a9b950ac 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/.ratings
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/.ratings
@@ -1,4 +1,4 @@
-DataRootElement.java d54ab629fc0dc9069586a3c7edfec0734e35dd86 YELLOW
-MetricDataManager.java f9e2d04edeb31f0af094d5ad92f6a573c1b90373 YELLOW
-MetricKey.java fa68a7e6025738534345708f13165acf014b4c8d YELLOW
-MetricTreeNode.java d89ef9aab7171a66ff7fc30c904e06f50d164ff9 YELLOW
+DataRootElement.java ab41ea9b52b095be3c03a85a6d1a3036b3026062 RED
+MetricDataManager.java dd2a265e4ac2738b5b68048111597902a763d79f RED
+MetricKey.java ac73424d340e52df84d6ad202d43d0cef9aeba78 RED
+MetricTreeNode.java 0e289fb25da429e11a931dec23d34204d7f12dd2 RED
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/DataRootElement.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/DataRootElement.java
index d54ab629fc0dc9069586a3c7edfec0734e35dd86..ab41ea9b52b095be3c03a85a6d1a3036b3026062 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/DataRootElement.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/DataRootElement.java
@@ -22,12 +22,12 @@ import org.fortiss.tooling.kernel.extension.data.ITopLevelElement;
/**
* Storage element corresponding to a {@Link IProjectRootElement}. It stores the last refresh time
* as {@link LocalDateTime}, its respective {@link ITopLevelElement} and the {@link MetricTreeNode}
- * storing the data
+ * storing the data.
*
* @author groh
*/
public class DataRootElement {
-
+ // TODO (SB): Comments start with a capital letter, end with a dot, and are free of typos.
/** the last time a metric tree was refreshed */
private LocalDateTime lastRefresh;
@@ -55,11 +55,13 @@ public class DataRootElement {
this.rootNode = rootNode;
}
- /** Returns lastRefresh. */
+ /** Returns {@link #lastRefresh}. */
public LocalDateTime getLastRefresh() {
return lastRefresh;
}
+ // TODO (SB): Link the field as I showed you above (5x). If you add an "#" in front of the field
+ // name, auto-complete works.
/** Sets lastRefresh. */
public void setLastRefresh(LocalDateTime lastRefresh) {
this.lastRefresh = lastRefresh;
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricDataManager.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricDataManager.java
index f9e2d04edeb31f0af094d5ad92f6a573c1b90373..dd2a265e4ac2738b5b68048111597902a763d79f 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricDataManager.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricDataManager.java
@@ -21,6 +21,8 @@ import java.util.Map;
import org.eclipse.emf.ecore.EObject;
import org.fortiss.tooling.kernel.model.IProjectRootElement;
+// TODO (SB): Rename class name to something like "MetricData". It is not a manager, but a data container.
+// TODO (SB): Remove af3 from the comment. We are in the kernel here!
/**
* Class containing all computed metrics for the current af3 instance.
*
@@ -28,12 +30,14 @@ import org.fortiss.tooling.kernel.model.IProjectRootElement;
*/
public class MetricDataManager {
+ // TODO (SB): Comments start with a capital letter and end with a dot.
/** a map to lookup the corresponding tree node for any {@link EObject} */
private Map<EObject, MetricTreeNode> treeNodeLookupTable;
/** a map to lookup the corresponding root tree node for a project */
private Map<IProjectRootElement, DataRootElement> dataRootElementMap;
+ // TODO (SB): Constructors are commented just with "Constructor." (one line).
/**
* Constructor: Creates a new instance of the MetricDataManager.
*/
@@ -42,6 +46,8 @@ public class MetricDataManager {
treeNodeLookupTable = new HashMap<>();
}
+ // TODO (SB): You can just use the following comment (avoids inconsistencies).
+ /** Getter for {@link #treeNodeLookupTable}. */
/**
* @return the lookup mapping between {@link EObject} and a {@link MetricTreeNode}.
*/
@@ -49,6 +55,7 @@ public class MetricDataManager {
return treeNodeLookupTable;
}
+ // TODO (SB): As above
/**
* @return the map between {@link IProjectRootElement} and the corresponding root
* {@link DataRootElement}.
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricKey.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricKey.java
index fa68a7e6025738534345708f13165acf014b4c8d..ac73424d340e52df84d6ad202d43d0cef9aeba78 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricKey.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricKey.java
@@ -27,6 +27,7 @@ public enum MetricKey {
//
// Metrics provided by the HierarchicElementProvider
+ // TODO (SB): All comments should end with a dot.
//
/** Unique ID of the element */
UNIQUE_ID(false),
@@ -37,6 +38,8 @@ public enum MetricKey {
NUMBER_OF_CONTAINED_ELEMENTS(false),
/** Key for the metric counting channels */
NUMBER_OF_CONNECTIONS(false),
+ // TODO (SB): This adds knowledge about the AF3 metamodel to the kernel. Create a ticket and a
+ // TODO(#xyz) for a cleaner solution.
/** Safety level of the element */
SAFETY_LEVEL(false, true),
/** How deeply nested the corresponding element is */
@@ -48,9 +51,7 @@ public enum MetricKey {
* recursively
*/
BETWEENESS_CENTRALITY_RECURSIVELY(false, 0.0),
- /**
- * The clustering coefficient of the element considering its neighbors
- */
+ /** The clustering coefficient of the element considering its neighbors. */
CLUSTERING_COEFFICIENT(false, 0.0),
/** How many constraints this element violates with severity error */
@@ -58,8 +59,12 @@ public enum MetricKey {
/** How many constraints this element violates with severity warning */
CONSTRAINT_VIOLATIONS_WARNING(false),
+ // TODO (SB): This adds knowledge about the AF3 metamodel to the kernel. Create a ticket and a
+ // TODO(#xyz) for a cleaner solution. (see above)
/** Number of allocated components to this task */
TASK_ALLOCATED_COMPONENTS(false),
+ // TODO (SB): This adds knowledge about the AF3 metamodel to the kernel. Create a ticket and a
+ // TODO(#xyz) for a cleaner solution. (see above)
/** How many constraints this element violates with severity warning */
EXECUTION_UNIT_ALLOCATED_TASKS(false),
@@ -99,17 +104,18 @@ public enum MetricKey {
/** Metrics which are a combination from the data of multiple nodes */
private static List<MetricKey> collectorKeys = new ArrayList<>();
- /**
- * Also see {@link MetricKey#collectorKeys}
- *
- * @return the keys which are combined from all children of a node
- */
+ /** Getter for {@link #collectorKeys}. */
public static List<MetricKey> getCollectorKeys() {
return collectorKeys;
}
- /** Used for populating the {@link MetricKey#getCollectorKeys()} Method */
+ /** Used for populating the {@link MetricKey#getCollectorKeys()} method. */
private boolean isCollectorKey;
+
+ // TODO (SB): In MetricKeyNode, there are int, double, and String metrics. However, here are
+ // only doubles and strings. What about marking the type of a MetricKey with a
+ // Class<? extends Number> field? In the constructor, you can check that only the admissible
+ // types Integer, Double, String are passed.
/** Determines if the corresponding value to this key is a string or not */
private boolean isStringValue;
/** Determines if the corresponding value to this key is a integer or not */
@@ -173,6 +179,9 @@ public enum MetricKey {
return isDoubleValue;
}
+ // TODO (SB): It would be less bloat and easier to read to do a lazy initialization of
+ // collectorKeys in getCollectorKeys(). I.e., do not initialize field with empty list, but
+ // do a null check and set collectorKeys when not already done.
static {
for(MetricKey key : MetricKey.values()) {
if(key.isCollectorKey) {
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricTreeNode.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricTreeNode.java
index d89ef9aab7171a66ff7fc30c904e06f50d164ff9..0e289fb25da429e11a931dec23d34204d7f12dd2 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricTreeNode.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricTreeNode.java
@@ -22,7 +22,7 @@ import java.util.Map;
import java.util.function.Consumer;
/**
- * A general class for storing all kinds of metric data in a hierarchical tree structure.
+ * A general class for storing metric data in a hierarchical tree structure.
* <p>
* Allows for efficient storage and retrieval of data.
*
@@ -33,6 +33,7 @@ public class MetricTreeNode {
/** stores the children of this tree node */
private List<MetricTreeNode> children;
+ // TODO (SB): Rename to integerMetrics (also getter). Same for doubles and strings
/** Map containing all metrics which are a Integer */
private Map<MetricKey, Integer> storedIntegers;
@@ -68,6 +69,8 @@ public class MetricTreeNode {
this.name = name;
}
+ // TODO (SB): In addition to renaming (see above), adjust comment: "the {@link Integer} metric
+ // map". (Hint: auto-completing types also works in comments).
/** @return the stored Integer map */
public Map<MetricKey, Integer> getStoredIntegers() {
return storedIntegers;
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/.ratings b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/.ratings
index c939dee612b5b481fe20a7410f35260dabd1b8a0..d882fa6e7e863cdaa40a5d822ab2a92e87469991 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/.ratings
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/.ratings
@@ -1,2 +1,2 @@
-IModelQualityService.java b530e32826340efaa3872e110d71e1c3a720dc07 YELLOW
-ModelQualityService.java a996770744711fb101f174b3421b4984ecdf60d8 YELLOW
+IModelQualityService.java 9a844024d62c79421e39d38699333587bec187d7 GREEN
+ModelQualityService.java 7d0021f1cd1281fa599f615b425bc9fd4e372f91 RED
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/IModelQualityService.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/IModelQualityService.java
index b530e32826340efaa3872e110d71e1c3a720dc07..9a844024d62c79421e39d38699333587bec187d7 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/IModelQualityService.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/IModelQualityService.java
@@ -27,7 +27,7 @@ import org.fortiss.tooling.kernel.extension.data.ITopLevelElement;
*/
public interface IModelQualityService {
/** Returns the service instance. */
- public static IModelQualityService getInstance() {
+ static IModelQualityService getInstance() {
return ModelQualityService.getInstance();
}
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/ModelQualityService.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/ModelQualityService.java
index a996770744711fb101f174b3421b4984ecdf60d8..7d0021f1cd1281fa599f615b425bc9fd4e372f91 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/ModelQualityService.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/ModelQualityService.java
@@ -16,6 +16,7 @@
package org.fortiss.tooling.ext.quality.service;
import static java.util.Collections.emptyList;
+import static java.util.stream.Collectors.groupingBy;
import java.io.IOException;
import java.security.NoSuchAlgorithmException;
@@ -29,7 +30,6 @@ import java.util.Map.Entry;
import java.util.Optional;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;
-import java.util.stream.Collectors;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
@@ -55,18 +55,18 @@ import org.fortiss.tooling.kernel.introspection.IIntrospectionItem;
import org.fortiss.tooling.kernel.introspection.IIntrospectiveKernelService;
import org.fortiss.tooling.kernel.model.IProjectRootElement;
import org.fortiss.tooling.kernel.service.IKernelIntrospectionSystemService;
-import org.fortiss.tooling.kernel.service.IMigrationService;
import org.fortiss.tooling.kernel.service.IPersistencyService;
import org.fortiss.tooling.kernel.service.base.EObjectAwareServiceBase;
import org.fortiss.tooling.kernel.service.listener.IPersistencyServiceListener;
import org.fortiss.tooling.kernel.utils.EcoreUtils;
/**
- * This class implements the {@link IMigrationService} interface.
+ * This class implements the {@link IModelQualityService}.
*
* @author blaschke
* @author groh
*/
+// TODO (SB): Change public to /* package */ to ensure use of IModelQualityService interface
public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider<? extends EObject>>
implements IIntrospectiveKernelService, IModelQualityService, IPersistencyServiceListener {
@@ -74,10 +74,13 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
private static final ModelQualityService INSTANCE = new ModelQualityService();
/** Returns singleton instance of the service. */
+ // TODO (SB): Change public to /* package */ to ensure use of IModelQualityService interface
public static ModelQualityService getInstance() {
return INSTANCE;
}
+ // TODO (SB): This service does not use the Eclipse extension points. Remove these 3 constants
+ // and have the respective methods return "null"
/** The connector extension point ID. */
private static final String EXTENSION_POINT_NAME =
"org.fortiss.tooling.ext.quality.metricService";
@@ -88,6 +91,8 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
/** The connector attribute name. */
private static final String ATTRIBUTE_NAME = "metricService";
+ // TODO (SB): Rename class name, this field, getter, etc. to something like "MetricData". It is
+ // not a manager, but a data container
/** Instance of the MetricDataManager. */
private MetricDataManager metricDataManagerInstance = new MetricDataManager();
@@ -99,10 +104,15 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
new ConcurrentLinkedQueue<ITopLevelElement>();
/** Stores the metrics collector job. */
- private final Job metricCollectorJob = new Job("Collecting metrics ...") {
+ private final Job metricCollectorJob = new Job("Collecting metrics...") {
@Override
protected IStatus run(IProgressMonitor monitor) {
+ // TODO (SB): Please check if the following comment that explains the loop logic is
+ // correct (otherwise consider changing to while (!queuedProcessableElements.isEmpty())
+ //
+ // Using a do-while-true loop with internal termination in order to ensure that the
+ // monitor is checked for cancellation at least once.
do {
if(monitor.isCanceled()) {
return Status.CANCEL_STATUS;
@@ -126,6 +136,10 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
@Override
public void scheduleMetricCollection(ITopLevelElement element) {
queuedProcessableElements.add(element);
+ // TODO (SB): Please check if the following comment is correct
+ // The Eclipse Job ensures that at most one instance of the metrics collection is executed.
+ // When a new element is added to the processing queue while a job is active, this current
+ // job will run longer in order to process it.
metricCollectorJob.schedule();
}
@@ -153,9 +167,9 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
@Override
public String getIntrospectionDescription() {
return getIntrospectionLabel() +
- "\n\nThis service allows the tracking of metrical development in modeling." +
- "\n the service outputs the current metric analysis of the model and stores" +
- "\n the data within a csv for later history analysis." +
+ "\n\nThis service allows to track the evolution of model metrics." +
+ "\n The service determines the current metrics of a model and stores" +
+ "\n them in a CSV file for later analysis of the modeling history." +
"\n\nThe service extension point is '" + EXTENSION_POINT_NAME + "'.";
}
@@ -169,9 +183,19 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
*/
private void performMetricCollection(ITopLevelElement topLevelElement)
throws NoSuchAlgorithmException, IOException {
+ // TODO (SB): Is the problem that metrics a duplicated when following (non-containment)
+ // EReferences?
+ // Is is only a problem when IProjectRoot elements (as for the allocation tables) are
+ // referenced, or a general problem? A more general solution would be to adapt the model
+ // traversal strategy.
+
// Sort root elements, as some metrics are saved into trees from other root elements
// To avoid these problems, the elements have to be processed in a certain order
+ // TODO (SB): In case the model traversal strategy cannot be changed, the implementation
+ // below has to be generalized such that no knowledge about the AF3 metamodel is injected
+ // into the kernel.
+
// Currently the only element affected by this is the allocation table, which is
// why push it to the end of the list
List<IProjectRootElement> rootElements = new ArrayList<>();
@@ -191,13 +215,11 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
}
return 0;
});
+
Map<IProjectRootElement, DataRootElement> updatedElements = new HashMap<>();
for(IProjectRootElement rootElement : rootElements) {
-
MetricTreeNode rootNode = new MetricTreeNode();
-
if(rootElement instanceof IHierarchicElement) {
-
IHierarchicElement firstElement = (IHierarchicElement)rootElement;
recursivlyCollectMetrics(rootNode, firstElement, 0);
} else {
@@ -212,8 +234,7 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
// Add the violated constraints to the corresponding tree node
for(Entry<? extends EObject, List<IConstraintViolation<? extends EObject>>> entry : contraintViolations
- .stream().collect(Collectors.groupingBy(IConstraintViolation::getSource))
- .entrySet()) {
+ .stream().collect(groupingBy(IConstraintViolation::getSource)).entrySet()) {
MetricTreeNode node =
metricDataManagerInstance.getTreeNodeLookupTable().get(entry.getKey());
@@ -228,6 +249,10 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
.filter(v -> v.getSeverity() == ESeverity.WARNING).count());
}
}
+
+ // TODO (SB): I would remember the current timestamp once (i.e., before the outermost
+ // for-loop). That way, all metrics of the current run will have the same timestamp. If
+ // there is a reason to provide a current timestamp, you should provide a comment
DataRootElement dataRootElement =
new DataRootElement(LocalDateTime.now(), topLevelElement, rootNode);
metricDataManagerInstance.getDataRootElementMap().put(rootElement, dataRootElement);
@@ -235,7 +260,7 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
metricDataManagerInstance.getTreeNodeLookupTable().put(rootElement, rootNode);
metricUpdateListeners.forEach(l -> l.metricUpdate(rootElement));
}
- // Store all currently saved metrics to the csv
+ // Store all currently saved metrics to the CSV file
CSVFileWriter.metricExtractionToCSV(updatedElements);
}
@@ -258,7 +283,7 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
node.getStoredIntegers().put(MetricKey.NESTING_LEVEL, recursionLevel);
// Check if any of the specifications is an IHierarchicElement
- // This is for example the case with an StateAutomaton
+ // This is for example the case for AF3 StateAutomatons
Optional<IHierarchicElement> hierarchicElementOptional = currentElement.getSpecifications()
.stream().filter(s -> s instanceof IHierarchicElement)
.map(s -> (IHierarchicElement)s).findAny();
@@ -271,7 +296,8 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
if(containedElements.size() == 1) {
// Add reference from the element to this, so the lookups works as expected
manager.getTreeNodeLookupTable().put(specificationElement, node);
- // Skip the specification element to get a more useful tree structure
+ // There is exactly one contained element. Skip the specification element to get a
+ // more useful tree structure
recursivlyCollectMetrics(node, containedElements.get(0), recursionLevel);
} else {
recursivlyCollectMetrics(node, specificationElement, recursionLevel);
@@ -279,7 +305,6 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
// Add reference from the element to this, so the lookups works as expected
manager.getTreeNodeLookupTable().put(currentElement, node);
} else {
-
// Iterate over all children and merge values
var nodeDoubles = node.getStoredDoubles();
var nodeIntegers = node.getStoredIntegers();
@@ -301,7 +326,9 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
}
manager.getTreeNodeLookupTable().put(currentElement, node);
-
+ // TODO (SB): Why is the GraphMetricsProvider a special case? If possible, we should
+ // turn it into an ordinary metrics provider. Otherwise, we should create a ticket for a
+ // refactoring.
GraphMetricsProvider.calculateBetweennessCentrality(currentElement, manager, false);
GraphMetricsProvider.calculateBetweennessCentrality(currentElement, manager, true);
node.getStoredDoubles().put(MetricKey.CLUSTERING_COEFFICIENT,
@@ -324,7 +351,8 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
List<IMetricProvider<? extends EObject>> providers = getAllMetricProviders(object);
for(IMetricProvider<? extends EObject> provider : providers) {
-
+ // TODO (SB): A nice solution would be to change getAllMetricProviders() to return a
+ // List<IMetricProvider<EObject>>. Can be postponed in a ticket with a TODO(#xyz) here
// The provider in the list has to have a compatible method for the actual type
// as we just collected providers with a compatible type
((IMetricProvider<EObject>)provider).collectMetrics(node, object);
@@ -395,6 +423,8 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
@Override
public void topLevelElementAdded(ITopLevelElement element) {
// Nothing done
+ // TODO (SB): I think this happens when the user copies a project to the storage view and
+ // "refresh"es. Is the "nothing done" intentional? Please check if my assumption is correct.
}
/** {@inheritDoc} */
@@ -409,6 +439,8 @@ public class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider
scheduleMetricCollection(element);
}
+ // TODO (SB): The MetricDataManager (or MetricData, after the suggested renaming) is used by
+ // other classes => Expose on the IModelQualityService interface.
/** @return the {@link MetricDataManager} for this service instance. */
public MetricDataManager getMetricDataManagerInstance() {
return metricDataManagerInstance;
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/.ratings b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/.ratings
index abec1e37a6eaeaabc9e2159f80a967b893569ce5..1af854e96f300936f6e33d7f1b817a581a429563 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/.ratings
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/.ratings
@@ -1,2 +1,2 @@
-CSVFileWriter.java dc1f73b071a2acd8fffed383c3528c55b34768f0 YELLOW
-ModelQualityStorageManager.java 66569ed1d4c3fd12b8f80e526aad70a981622d25 YELLOW
+CSVFileWriter.java a779fce240c25aae1b72dfd11d27b74b0b21d599 RED
+ModelQualityStorageManager.java 8293f17743bdc85e2595eae99b978ed868bd029b RED
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/CSVFileWriter.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/CSVFileWriter.java
index dc1f73b071a2acd8fffed383c3528c55b34768f0..a779fce240c25aae1b72dfd11d27b74b0b21d599 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/CSVFileWriter.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/CSVFileWriter.java
@@ -15,6 +15,8 @@
+--------------------------------------------------------------------------*/
package org.fortiss.tooling.ext.quality.storage;
+import static java.lang.Integer.toHexString;
+
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
@@ -39,17 +41,20 @@ import java.util.stream.Stream;
import org.fortiss.tooling.ext.quality.data.DataRootElement;
import org.fortiss.tooling.ext.quality.data.MetricKey;
+import org.fortiss.tooling.kernel.extension.data.ITopLevelElement;
import org.fortiss.tooling.kernel.model.IProjectRootElement;
/**
- * Allows the export of metrics to a csv file.
+ * Allows the export of metrics to a CSV file.
*
* @author groh
*/
public class CSVFileWriter {
+ // TODO (SB): Comment is unclear. What is the default CSV file (I thought there are per-project
+ // metric files now)? What do you mean by the conversion?
/**
- * Method to write the data into the default csv.
+ * Method to write the data into the default CSV file.
* <p>
* It will always write to the same file, if it is already present it will attempt to read the
* keys and convert them into {@link MetricKey} and then write these keys in the ordering that
@@ -58,7 +63,7 @@ public class CSVFileWriter {
* was exported the first time.
*
* @param map
- * to convert into csv
+ * to save into CSV file
* @throws IOException
* @throws NoSuchAlgorithmException
*
@@ -71,22 +76,33 @@ public class CSVFileWriter {
return;
}
+ // TODO(SB): Move functionality to IModelQualityService (to which the directory is
+ // configured during startup, see comments in ModelQualityStorageManager)
// Ensure the folder exists
if(!ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR.exists()) {
ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR.mkdirs();
}
- // path to csv file
+ // path to CSV file
+ // TODO (SB): What about the following (avoids creating an array):
+ // Check above is that iterator.next() returns at least one value
+ // DataRootElement firstValue = map.entrySet().iterator().next().getValue();
Collection<DataRootElement> values = map.values();
DataRootElement firstValue = values.toArray(new DataRootElement[0])[0];
- String projectName = firstValue.getTopLevelElement().getSaveableName();
+ ITopLevelElement topLevelElement = firstValue.getTopLevelElement();
+ String projectName = topLevelElement.getSaveableName();
+
+ // TODO: the quality file name should be determined without knowledge of the AF3 file
+ // extension. Maybe FilenameUtils.getBaseName(filename) is useful?
Path path = new File(ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR, projectName
.replace(ModelQualityStorageManager.AF3_PRJ_SUFFIX, ModelQualityStorageManager.CSV))
.toPath();
- Path af3File = new File(ModelQualityStorageManager.MODEL_PROJECT_DIR,
- firstValue.getTopLevelElement().getSaveableName()).toPath();
- String projectHash = computeGitHash(af3File);
+ // TODO(SB): This assumes that the file has been saved to disk.
+ // Add topLevelElement.doSave(new NullProgressMonitor());
+ Path modelFile = new File(ModelQualityStorageManager.MODEL_PROJECT_DIR,
+ topLevelElement.getSaveableName()).toPath();
+ String projectHash = computeGitHash(modelFile);
List<String> allKeys = new ArrayList<>();
boolean createNewIndex = true;
@@ -100,6 +116,8 @@ public class CSVFileWriter {
createNewIndex = false;
}
} catch(IOException e) {
+ // In case there is an error, writeMetricsToFile() will provide the default keys.
+ // Therefore this exception can be ignored (and just logged).
e.printStackTrace();
}
}
@@ -107,7 +125,7 @@ public class CSVFileWriter {
}
/**
- * Function to write the specified metrics into a csv file.
+ * Function to write the specified metrics into a CSV file.
*
* @param map
* of the metrics which are written into the file
@@ -120,7 +138,7 @@ public class CSVFileWriter {
* @param projectName
* the name of the base fileProject for this map currently in the focus
* @param projectHash
- * the af3 file hash value saved for version identification
+ * the model file hash value saved for version identification
*/
private static void writeMetricsToFile(Map<IProjectRootElement, DataRootElement> map,
List<String> allKeys, Path path, boolean createNewIndex, String projectName,
@@ -128,7 +146,12 @@ public class CSVFileWriter {
try(var writer = new BufferedWriter(new FileWriter(path.toFile(), true))) {
if(createNewIndex) {
- // Create new index and write it into the first line of the file
+ // TODO (SB): It would be save to start the (currently 6) non-value keys with the
+ // same (reserved) prefix. That way, we can later change the strategy to distinguish
+ // them for the value keys. The implementation below to assume that there are
+ // currently 6 non-value keys is fine.
+
+ // Create new index and write it into the first line of the file.
allKeys.add("timestamp");
allKeys.add("project_name");
allKeys.add("project_hash");
@@ -144,6 +167,7 @@ public class CSVFileWriter {
writer.write(String.join(",", allKeys));
writer.write("\n");
}
+
// Remove first 6 keys as they are not provided by the provider
List<String> valueKeys = allKeys.subList(6, allKeys.size());
@@ -170,6 +194,7 @@ public class CSVFileWriter {
// Collect all values from provider in the correct order and combine
var values = Stream.concat(startStream, valueKeys.stream()
.map(String::toUpperCase).map(MetricKey::valueOf).map(k -> {
+ // TODO (SB): Always use braces for if. Avoids future problems :-)
if(k.isStringValue())
return strings.get(k);
if(k.isDoubleValue())
@@ -191,8 +216,8 @@ public class CSVFileWriter {
}
/**
- * Function to generate the hash for a fileProject to later on identify the versions with the
- * saved metrics.
+ * Function to generate the hash for the model in order to later on match versions of the file
+ * project with saved metrics (assuming both are under version control using Git).
*/
private static String computeGitHash(Path filePath)
throws NoSuchAlgorithmException, IOException {
@@ -206,7 +231,7 @@ public class CSVFileWriter {
private static String bytesToHex(byte[] hash) {
StringBuilder hexString = new StringBuilder(2 * hash.length);
for(byte b : hash) {
- String hex = Integer.toHexString(0xff & b);
+ String hex = toHexString(0xff & b);
if(hex.length() == 1) {
hexString.append('0');
}
diff --git a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/ModelQualityStorageManager.java b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/ModelQualityStorageManager.java
index 66569ed1d4c3fd12b8f80e526aad70a981622d25..8293f17743bdc85e2595eae99b978ed868bd029b 100644
--- a/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/ModelQualityStorageManager.java
+++ b/org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/ModelQualityStorageManager.java
@@ -21,33 +21,46 @@ import java.io.File;
import org.eclipse.core.resources.IWorkspaceRoot;
+// TODO (SB): This class is not really a manager, but a collection of constants.
+// TODO (SB): This class makes the quality calculation AF3 specific. The suggested
+// resolution is to distribute the information to other classes (as below).
+// Possibly postpone this in a dedicated ticket, and reference it here (TODO (#xyz))
/**
- * Manages the directory of the csv files.
+ * Manages the directory of the CSV files.
*
* @author groh
*/
public class ModelQualityStorageManager {
+ // TODO(SB): Move to org.fortiss.af3.project (e.g., create sister class of AF3Project (e.g.,
+ // AF3Metrics), and configure the IModelQualityService from there with the directory name, etc.)
/** The name of the general quality plugin directory. */
public static final String AF3_QUALITY_DIRECTORY_NAME = "AF3-Model-Quality-Directory";
+ // TODO(SB): Remove! The single source of truth for this is
+ // org.fortiss.af3.project.utils.FileUtils
/** The name of the general project directory. */
public static final String AF3_PROJECT_DIRECTORY_NAME = "AF3-Project-Directory";
+ // TODO(SB): Remove (unneeded magic)
/** The current workspace root. */
public static final IWorkspaceRoot WORKSPACE_ROOT = getWorkspace().getRoot();
+ // TODO(SB): Remove, see suggested AF3Metrics class above
/** {@link File} representing the directory where all data is saved. */
public static final File MODEL_QUALITY_PROJECT_DIR =
new File(WORKSPACE_ROOT.getLocation() + File.separator + AF3_QUALITY_DIRECTORY_NAME);
+ // TODO(SB): Remove, see suggested AF3Metrics class above
/** {@link File} representing the directory where all data is saved. */
public static final File MODEL_PROJECT_DIR =
new File(WORKSPACE_ROOT.getLocation() + File.separator + AF3_PROJECT_DIRECTORY_NAME);
+ // TODO (SB): Remove! The single source of truth for this is org.fortiss.af3.project.AF3Project
/** Defines the suffix of AF3 project files. */
public static final String AF3_PRJ_SUFFIX = "af3_23";
+ // TODO(SB): Move as (private) constant CSV_SUFFIX to CSVFileWriter
/** Suffix of the file in which the data is written. */
public static final String CSV = "csv";
}