Review (one RED, rest GREEN with some TODOs linking to issues)

Issue-Ref: 4310
Issue-Url: af3#4310



Signed-off-by: Simon Barner <barner@fortiss.org>

org.fortiss.tooling.ext.quality.ui/src/org/fortiss/tooling/ext/quality/ui/view/fx/.ratings

 IModelQualityViewPart.java 708f8089645df12098ea67190805cce343045d2e GREEN
-ModelQualityFXController.java 64c5fb2ca8cca77982b5a20695efcf2b1442045e RED
+ModelQualityFXController.java 91a82ea532fda6ef6b6db0430cece98016add60f RED
 ModelQualityFXViewPart.java 9cfc7f60a86ea5b915c726b16712f8be7dec2c5f GREEN

org.fortiss.tooling.ext.quality.ui/src/org/fortiss/tooling/ext/quality/ui/view/fx/ModelQualityFXController.java

@@ -125,7 +125,7 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 
 	/**
 	 * The last {@link EObject} which the user has clicked. Might be null when no or an invalid
-	 * Object is selected.
+	 * object is selected.
 	 */
 	private EObject lastSelectedElement;
 
@@ -167,8 +167,8 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 			return;
 		}
 
-		IModelQualityService mqs = IModelQualityService.getInstance();
-		var rootDataElement = mqs.getMetricData().getDataRootElementMap().get(currentRootElement);
+		MetricData metricData = IModelQualityService.getInstance().getMetricData();
+		var rootDataElement = metricData.getDataRootElementMap().get(currentRootElement);
 
 		if(rootDataElement == null) {
 			bottomText.setText(
@@ -328,7 +328,8 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 	private SpiderChartViewer buildSpiderChart() {
 		// Calculate the intersection of the keys, i.e. a set of keys which have a non-null value in
 		// all elements of the lastViewedElements list
-		// TODO (SB): Add check that lastViewedElements is non empty, and add a corresonding comment
+		// TODO (SB): Add check that lastViewedElements is non empty, and add a corresponding
+		// comment
 		// to the .get(0) statement
 		Set<MetricKey> keysIntersection = lastViewedElements.get(0).getValue();
 		for(var p : lastViewedElements) {
@@ -395,9 +396,7 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 		// Configure legend
 		LegendStyle legendStyle = new LegendStyle(false, 5, BLACK_VERDANA_12PT);
 		chartStyle.setLegendStyle(legendStyle);
-
 		chartStyle.setAxisSegments(4);
-
 		chartStyle.setBackgroundFillStyle(new FillStyle(LIGHTGRAY));
 		chartStyle.setBackgroundLineStyle(new LineStyle(DARKGRAY));
 
@@ -414,8 +413,8 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 				return;
 			}
 
-			IModelQualityService mqs = IModelQualityService.getInstance();
-			var rootDataElement = mqs.getMetricData().getDataRootElementMap().get(updatedElement);
+			MetricData metricData = IModelQualityService.getInstance().getMetricData();
+			var rootDataElement = metricData.getDataRootElementMap().get(updatedElement);
 			if(rootDataElement != null) {
 				bottomText.setText("Metrics last updated: " +
 						timeFormat.format(rootDataElement.getLastRefresh()) +
@@ -440,8 +439,8 @@ public class ModelQualityFXController extends CompositeFXControllerBase<SplitPan
 
 		// Create set of keys which store values in the ndoe
 		Set<MetricKey> keys = new HashSet<>();
-		keys.addAll(node.getStoredDoubles().keySet());
-		keys.addAll(node.getStoredIntegers().keySet());
+		keys.addAll(node.getDoubleMetrics().keySet());
+		keys.addAll(node.getIntegerMetrics().keySet());
 
 		// Check if the node is already part of the list, and skip adding it if it is
 		if(lastViewedElements.stream().allMatch(p -> p.getKey() != node)) {

org.fortiss.tooling.ext.quality/html/developer/.ratings

-documentation.html b66c2bfb495047a8e4b680b02231ab2fdb34cec3 YELLOW
+documentation.html b66c2bfb495047a8e4b680b02231ab2fdb34cec3 GREEN

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/.ratings

-GraphMetricsProvider.java d18cff37dcb103a447cead64e1c2b00866996b3c YELLOW
-HierarchicElementMetricProvider.java e76e846d76c6ba8782a20d9c9f9c6d57eaaa416e YELLOW
+GraphMetricsProvider.java ce70c838b887df0bdf776f462b23cf594c700822 GREEN
+HierarchicElementMetricProvider.java 966b3aeef92475034fc35e68ba5344688988b88a GREEN
 IMetricProvider.java 99fc8993b0e65b2f8757978eeb0481d912f5608c GREEN
 IMetricUpdateListener.java c24dc7c0f282623bbf1eefac1fbbb6752c97ddf0 GREEN
-ModelQualityActivator.java 6d5f0794aa48670cf45fb405bd7641bce6c1fec4 YELLOW
+ModelQualityActivator.java 6d5f0794aa48670cf45fb405bd7641bce6c1fec4 GREEN

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/GraphMetricsProvider.java

@@ -15,6 +15,8 @@
 +--------------------------------------------------------------------------*/
 package org.fortiss.tooling.ext.quality;
 
+import static org.fortiss.tooling.kernel.utils.EcoreUtils.pickInstanceOf;
+
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -25,8 +27,6 @@ import java.util.Queue;
 import java.util.Set;
 import java.util.Stack;
 
-import static org.fortiss.tooling.kernel.utils.EcoreUtils.pickInstanceOf;
-
 import org.eclipse.emf.common.util.EList;
 import org.fortiss.tooling.base.model.base.EntryConnectorBase;
 import org.fortiss.tooling.base.model.base.ExitConnectorBase;
@@ -35,6 +35,7 @@ import org.fortiss.tooling.base.model.element.IConnector;
 import org.fortiss.tooling.base.model.element.IHierarchicElement;
 import org.fortiss.tooling.ext.quality.data.MetricData;
 import org.fortiss.tooling.ext.quality.data.MetricKey;
+import org.fortiss.tooling.ext.quality.data.MetricTreeNode;
 
 /**
  * Supplies methods to compute certain graph metrics.
@@ -42,7 +43,7 @@ import org.fortiss.tooling.ext.quality.data.MetricKey;
  * @author groh
  */
 
-// TODO (#4332) Restructure GraphMetricsProvider in quality Plugin
+// TODO (#4332) Restructure GraphMetricsProvider
 public class GraphMetricsProvider {
 
 	/**
@@ -171,9 +172,11 @@ public class GraphMetricsProvider {
 
 	/**
 	 * Calculates the clustering coefficient based on the neighborhood.
-	 * https://en.wikipedia.org/wiki/Clustering_coefficient
-	 * Clustering coefficient C for a node V is counting the triangles divided by the "number of neighbors of
-	 * V" times "number of neighbors of V - 1" which results in the potential number of maximum links between the nodes
+	 * See, e.g., https://en.wikipedia.org/wiki/Clustering_coefficient
+	 * Clustering coefficient C for a node V is counting the triangles divided by the "number of
+	 * neighbors of "V" times "number of neighbors of V - 1" which results in the potential number
+	 * of maximum links between the nodes
+	 * 
 	 * @param element
 	 *            of which the coefficient shall be calculated
 	 * @return value of the coefficient
@@ -202,7 +205,7 @@ public class GraphMetricsProvider {
 
 	/**
 	 * Calculates and saves the betweenness centrality of all elements contained inside the provided
-	 * scopeElement. Described for example in https://en.wikipedia.org/wiki/Betweenness_centrality
+	 * scopeElement. See, e.g., https://en.wikipedia.org/wiki/Betweenness_centrality
 	 * 
 	 * @param scopeElement
 	 *            the scope of this calculation
@@ -218,9 +221,9 @@ public class GraphMetricsProvider {
 
 		// Abort if no elements are found
 		if(scopeElement.getContainedElements().isEmpty()) {
-			return; 
+			return;
 		}
-		
+
 		Set<IHierarchicElement> graphNodes = getLocalGraphView(scopeElement, recursively);
 		// Initialize the betweenness values for all nodes to zero
 		Map<IHierarchicElement, Double> betweenness = new HashMap<>();
@@ -277,8 +280,8 @@ public class GraphMetricsProvider {
 		MetricKey key = recursively ? MetricKey.BETWEENESS_CENTRALITY_RECURSIVELY
 				: MetricKey.BETWEENESS_CENTRALITY;
 		for(IHierarchicElement child : scopeElement.getContainedElements()) {
-			metricData.getTreeNodeLookupTable().get(child).getStoredDoubles().put(key,
-					betweenness.get(child));
+			MetricTreeNode node = metricData.getTreeNodeLookupTable().get(child);
+			node.getDoubleMetrics().put(key, betweenness.get(child));
 		}
 	}
 
@@ -316,7 +319,8 @@ public class GraphMetricsProvider {
 			stack.push(v);
 
 			// Iterate over all connectors going out from the element
-			for(ExitConnectorBase exitConnectors : pickInstanceOf(ExitConnectorBase.class, v.getConnectors())) {
+			for(ExitConnectorBase exitConnectors : pickInstanceOf(ExitConnectorBase.class,
+					v.getConnectors())) {
 				// Iterate over all connections
 				for(IConnection outgoingConnection : exitConnectors.getOutgoing()) {
 
@@ -353,7 +357,7 @@ public class GraphMetricsProvider {
 							if(targetDistance < 0) {
 								queue.add(targetElement);
 								distance.put(targetElement, vDistance + 1);
-							}else if(targetDistance == vDistance + 1) {
+							} else if(targetDistance == vDistance + 1) {
 								dependency.put(targetElement,
 										dependency.get(targetElement) + dependency.get(v));
 								predecessor.get(targetElement).add(v);

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/HierarchicElementMetricProvider.java

@@ -19,7 +19,6 @@ import static org.fortiss.tooling.kernel.utils.EcoreUtils.pickInstanceOf;
 
 import org.fortiss.tooling.base.model.base.EntryConnectorBase;
 import org.fortiss.tooling.base.model.base.ExitConnectorBase;
-
 import org.fortiss.tooling.base.model.element.IHierarchicElement;
 import org.fortiss.tooling.ext.quality.data.MetricKey;
 import org.fortiss.tooling.ext.quality.data.MetricTreeNode;
@@ -36,33 +35,32 @@ public class HierarchicElementMetricProvider implements IMetricProvider<IHierarc
 	@Override
 	public void collectMetrics(MetricTreeNode node, IHierarchicElement currentElement) {
 
-		var intMetrics = node.getStoredIntegers();
+		var intMetrics = node.getIntegerMetrics();
 		// Only collect these metrics if the name is null, in which case they have not been
 		// collected before
 		// This is used in some cases to combine metrics from two different IHierarchicElement
 		if(node.getName() == null) {
 
 			// Metrics are always set to default value, as these metric are combined later
-			String comment;
 			if(currentElement instanceof INamedCommentedElement) {
 				intMetrics.put(MetricKey.NUMBER_OF_TOTAL_COMMENTABLE_ELEMENTS, 1);
-				comment =((INamedCommentedElement)currentElement).getComment();
-				if(comment != "") {
+				INamedCommentedElement nce = (INamedCommentedElement)currentElement;
+				String comment = nce.getComment();
+				if(comment != null && comment != "") {
 					intMetrics.put(MetricKey.NUMBER_OF_TOTAL_COMMENTED_ELEMENTS, 1);
 				}
-				node.setName(((INamedCommentedElement)currentElement).getName());
-			}else {
+				node.setName(nce.getName());
+			} else {
 				intMetrics.put(MetricKey.NUMBER_OF_TOTAL_COMMENTABLE_ELEMENTS, 0);
-				comment = null;
 				intMetrics.put(MetricKey.NUMBER_OF_TOTAL_COMMENTED_ELEMENTS, 0);
 				node.setName("No Name existant");
 			}
-			
+
 			if(currentElement instanceof IIdLabeled) {
-				intMetrics.put(MetricKey.UNIQUE_ID,((IIdLabeled)currentElement).getId());
-			}else {
+				intMetrics.put(MetricKey.UNIQUE_ID, ((IIdLabeled)currentElement).getId());
+			} else {
 				intMetrics.put(MetricKey.UNIQUE_ID, -1);
-			}			
+			}
 		}
 
 		// connector metrics
@@ -71,10 +69,10 @@ public class HierarchicElementMetricProvider implements IMetricProvider<IHierarc
 		intMetrics.put(MetricKey.NUMBER_OF_CONTAINED_ELEMENTS,
 				currentElement.getContainedElements().size());
 		intMetrics.put(MetricKey.NUMBER_OF_CONNECTIONS, currentElement.getConnections().size());
-		
+
 		// depth metrics
 		intMetrics.put(MetricKey.NUMBER_OF_TOTAL_CONNECTORS, connectors.size());
-		
+
 		var entry_connectors = pickInstanceOf(EntryConnectorBase.class, connectors);
 		var exit_connectors = pickInstanceOf(ExitConnectorBase.class, connectors);
 		intMetrics.put(MetricKey.NUMBER_OF_TOTAL_ENTRY_CONNECTORS, entry_connectors.size());
@@ -82,12 +80,12 @@ public class HierarchicElementMetricProvider implements IMetricProvider<IHierarc
 
 		intMetrics.put(MetricKey.NUMBER_OF_TOTAL_ELEMENTS, 1);
 
-		if (currentElement.getContainedElements().isEmpty()) {
+		if(currentElement.getContainedElements().isEmpty()) {
 			intMetrics.put(MetricKey.NUMBER_OF_TOTAL_LEAF_ELEMENTS, 0);
-		}else {
+		} else {
 			intMetrics.put(MetricKey.NUMBER_OF_TOTAL_LEAF_ELEMENTS, 1);
 		}
 
-		//graph metrics
-	}	
+		// graph metrics
+	}
 }

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/.ratings

-DataRootElement.java e598763b5199c4810da191623a07a9edeb154a84 YELLOW
-MetricData.java 03fd1cf0477a42f1f50c899c180035498d21faa3 YELLOW
-MetricKey.java 81ba35f8537c4cb9c0f487a2bfafdb65f9d6c3c0 YELLOW
-MetricTreeNode.java 8eb3c6761a3af482215d48e1bada53415d4ba8e4 YELLOW
+DataRootElement.java 7b655236c155e3a4473301aa8c3ecfe769cf071c GREEN
+MetricData.java b3c1a395cca29ced5845b789d76ee2c965ec66f6 GREEN
+MetricKey.java d78026f0c68aeeff0cc758bbd76fbb84da65afd6 GREEN
+MetricTreeNode.java 88ddbdfc62fb8efee71aa3ee44257d1a1a582fe6 GREEN

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/DataRootElement.java

@@ -30,13 +30,13 @@ public class DataRootElement {
 	/** The last time a metric tree was refreshed. */
 	private LocalDateTime lastRefresh;
 
-	/** the last time the metric tree was refreshed */
+	/** The last time the metric tree was refreshed. */
 	private ITopLevelElement topLevelElement;
 
-	/** Root {@link MetricTreeNode} for the metric tree containg all data */
+	/** Root {@link MetricTreeNode} for the metric tree containing all data. */
 	private MetricTreeNode rootNode;
 
-	/**Constructor. */
+	/** Constructor. */
 	public DataRootElement(LocalDateTime lastRefresh, ITopLevelElement topLevelElement,
 			MetricTreeNode rootNode) {
 		super();
@@ -65,12 +65,12 @@ public class DataRootElement {
 		this.topLevelElement = topLevelElement;
 	}
 
-	/** Getter for {@link #MetricTreeNode}. */
+	/** Getter for {@link #rootNode}. */
 	public MetricTreeNode getRootNode() {
 		return rootNode;
 	}
 
-	/** Setter for {@link #MetricTreeNode}. */
+	/** Setter for {@link #rootNode}. */
 	public void setRootNode(MetricTreeNode metricTreeNode) {
 		this.rootNode = metricTreeNode;
 	}

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricData.java

@@ -22,7 +22,7 @@ import org.eclipse.emf.ecore.EObject;
 import org.fortiss.tooling.kernel.model.IProjectRootElement;
 
 /**
- * Class storing the metrics computed for the all suitable elemnets.
+ * Class storing the metrics computed for the all suitable elements.
  * 
  * @author groh
  */
@@ -34,7 +34,7 @@ public class MetricData {
 	/** A map to lookup the corresponding root tree node for a project. */
 	private Map<IProjectRootElement, DataRootElement> dataRootElementMap;
 
-	/** Constructor.*/
+	/** Constructor. */
 	public MetricData() {
 		dataRootElementMap = new HashMap<>();
 		treeNodeLookupTable = new HashMap<>();
@@ -45,7 +45,7 @@ public class MetricData {
 		return treeNodeLookupTable;
 	}
 
-	/** Getter for {@link #DataRootElementMap}. */
+	/** Getter for {@link #dataRootElementMap}. */
 	public Map<IProjectRootElement, DataRootElement> getDataRootElementMap() {
 		return dataRootElementMap;
 	}

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricKey.java

@@ -21,6 +21,7 @@ import java.util.List;
 /**
  * Enum class representing a key which can be used to store a respective metric.
  * Metrics provided by the HierarchicElementProvider.
+ * 
  * @author groh
  */
 public enum MetricKey {
@@ -34,7 +35,7 @@ public enum MetricKey {
 	NUMBER_OF_CONTAINED_ELEMENTS(false),
 	/** Key for the metric counting channels. */
 	NUMBER_OF_CONNECTIONS(false),
-	// TODO (#4333): 
+	// TODO (#4333):
 	/** Safety level of the element. */
 	SAFETY_LEVEL(false, true),
 	/** How deeply nested the corresponding element is. */
@@ -73,9 +74,14 @@ public enum MetricKey {
 	NUMBER_OF_TOTAL_COMMENTABLE_ELEMENTS(true),
 	/** Key for the metric counting the total amount of elements which are commented. */
 	NUMBER_OF_TOTAL_COMMENTED_ELEMENTS(true),
-	/** Key for the metric counting the total amount of elements which do not contain other elements. */
+	/**
+	 * Key for the metric counting the total amount of elements which do not contain other elements.
+	 */
 	NUMBER_OF_TOTAL_LEAF_ELEMENTS(true),
-	/** Key for the metric counting of the total amount of constant values in all code specifications. */
+	/**
+	 * Key for the metric counting of the total amount of constant values in all code
+	 * specifications.
+	 */
 	NUMBER_OF_TOTAL_CODE_CONSTANTS(true),
 	/** Cyclomatic complexity number as sum over all components. */
 	NUMBER_OF_TOTAL_CODE_CYCLOMATIC_COMPLEXITY(true),
@@ -89,25 +95,24 @@ public enum MetricKey {
 
 	/** Getter for {@link #collectorKeys}. */
 	public static List<MetricKey> getCollectorKeys() {
-		   if (collectorKeys == null) {
-	            collectorKeys = new ArrayList<>();
-	            for (MetricKey key : MetricKey.values()) {
-	                if (key.isCollectorKey) {
-	                    collectorKeys.add(key);
-	                }
-	            }
-	        }
-	        return collectorKeys;
+		if(collectorKeys == null) {
+			collectorKeys = new ArrayList<>();
+			for(MetricKey key : MetricKey.values()) {
+				if(key.isCollectorKey) {
+					collectorKeys.add(key);
+				}
+			}
+		}
+		return collectorKeys;
 	}
 
 	/** 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
+	// TODO (4336): In MetricTreeNode, 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.
-	// TODO (#4333)
 	/** 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 */

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/data/MetricTreeNode.java

@@ -69,17 +69,17 @@ public class MetricTreeNode {
 	}
 
 	/** Getter for {@link #integerMetrics}. */
-	public Map<MetricKey, Integer> getStoredIntegers() {
+	public Map<MetricKey, Integer> getIntegerMetrics() {
 		return integerMetrics;
 	}
 
 	/** Getter for {@link #doubleMetrics}. */
-	public Map<MetricKey, Double> getStoredDoubles() {
+	public Map<MetricKey, Double> getDoubleMetrics() {
 		return doubleMetrics;
 	}
 
 	/** Getter for {@link #stringMetrics}. */
-	public Map<MetricKey, String> getStoredStrings() {
+	public Map<MetricKey, String> getStringMetrics() {
 		return stringMetrics;
 	}
 

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/.ratings

-IModelQualityService.java e474e51bec8ce03209b69a40f3433a070bd73daa YELLOW
-ModelQualityService.java 4a620f5c7732069d78b0e37e051ff99f4732642e YELLOW
+IModelQualityService.java db237db72cf1a0905fa82b1dfd799f830d092a10 GREEN
+ModelQualityService.java dc65c5cc1f06971db96d62aed4831b137929c789 GREEN

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/IModelQualityService.java

@@ -19,7 +19,6 @@ import org.eclipse.emf.ecore.EObject;
 import org.fortiss.tooling.ext.quality.IMetricProvider;
 import org.fortiss.tooling.ext.quality.IMetricUpdateListener;
 import org.fortiss.tooling.ext.quality.data.MetricData;
-import org.fortiss.tooling.ext.quality.storage.ModelQualityStorageManager;
 import org.fortiss.tooling.kernel.extension.data.ITopLevelElement;
 
 /**
@@ -32,7 +31,7 @@ public interface IModelQualityService {
 	static IModelQualityService getInstance() {
 		return ModelQualityService.getInstance();
 	}
-	
+
 	/** Starting the service. */
 	void startService();
 
@@ -44,8 +43,8 @@ public interface IModelQualityService {
 	void scheduleMetricCollection(ITopLevelElement top);
 
 	/** Returns the {@link MetricData} for the Service. */
-	MetricData getMetricData();	
-	
+	MetricData getMetricData();
+
 	/**
 	 * Registers the provided {@link IMetricUpdateListener} to be called when the metrics are
 	 * updated.
@@ -54,12 +53,7 @@ public interface IModelQualityService {
 	 *            to be registered
 	 */
 	void registerMetricUpdateListener(IMetricUpdateListener listener);
-	
+
 	/** Creating the quality folder to store the metrics in. */
-	static void createQualityFolder() {
-		if(!ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR.exists()) {
-			ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR.mkdirs();
-		}
-	}	
-	
+	void createQualityFolder();
 }

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/service/ModelQualityService.java

@@ -16,13 +16,14 @@
 package org.fortiss.tooling.ext.quality.service;
 
 import static java.util.Collections.emptyList;
+import static java.util.Collections.sort;
 import static java.util.stream.Collectors.groupingBy;
+import static org.fortiss.tooling.kernel.utils.KernelModelElementUtils.getRootElements;
 
 import java.io.IOException;
 import java.security.NoSuchAlgorithmException;
 import java.time.LocalDateTime;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -47,6 +48,7 @@ import org.fortiss.tooling.ext.quality.data.MetricData;
 import org.fortiss.tooling.ext.quality.data.MetricKey;
 import org.fortiss.tooling.ext.quality.data.MetricTreeNode;
 import org.fortiss.tooling.ext.quality.storage.CSVFileWriter;
+import org.fortiss.tooling.ext.quality.storage.ModelQualityStorageManager;
 import org.fortiss.tooling.kernel.extension.data.IConstraintViolation;
 import org.fortiss.tooling.kernel.extension.data.IConstraintViolation.ESeverity;
 import org.fortiss.tooling.kernel.extension.data.ITopLevelElement;
@@ -59,7 +61,6 @@ import org.fortiss.tooling.kernel.service.IKernelIntrospectionSystemService;
 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 IModelQualityService}.
@@ -67,7 +68,8 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
  * @author blaschke
  * @author groh
  */
-/* package */ class ModelQualityService extends EObjectAwareServiceBase<IMetricProvider<? extends EObject>>
+/* package */ class ModelQualityService
+		extends EObjectAwareServiceBase<IMetricProvider<? extends EObject>>
 		implements IIntrospectiveKernelService, IModelQualityService, IPersistencyServiceListener {
 
 	/** The singleton instance. */
@@ -78,7 +80,7 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 		return INSTANCE;
 	}
 
-	/** Instance of the MetricDataManager. */
+	/** Collected metrics. */
 	private MetricData metricDataContainer = new MetricData();
 
 	/** List of {@link IMetricUpdateListener} which are called when the metrics are updated */
@@ -109,7 +111,7 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 					e.printStackTrace();
 				} catch(IOException e) {
 					e.printStackTrace();
-				} catch (CoreException e) {
+				} catch(CoreException e) {
 					e.printStackTrace();
 				}
 			} while(true);
@@ -153,7 +155,7 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 				"\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 'not existant'.";
+				"\n\nThe service extension point is 'not existent'.";
 	}
 
 	/**
@@ -163,35 +165,19 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 	 *            the {@link ITopLevelElement} on which the analysis should be performed
 	 * @throws IOException
 	 * @throws NoSuchAlgorithmException
-	 * @throws CoreException 
+	 * @throws CoreException
 	 */
 	private void performMetricCollection(ITopLevelElement topLevelElement)
 			throws NoSuchAlgorithmException, IOException, CoreException {
-		// TODO (#4333): 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 (#4333): 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<>();
-		rootElements.addAll(EcoreUtils.getChildrenWithType(topLevelElement.getRootModelElement(),
-				IProjectRootElement.class));
+		List<IProjectRootElement> rootElements =
+				getRootElements(topLevelElement.getRootModelElement(), IProjectRootElement.class);
 
+		// TODO(#XXX): Remove below workaround that makes assumptions on the overall structure of
+		// the model, and its implementation.
 		String allocationTableClassName =
 				"org.fortiss.af3.allocation.model.impl.AllocationTableCollectionImpl";
-
-		// Sort with custom comparator which pushes the object back if it matches the above class
-		// name
-		Collections.sort(rootElements, (r1, r2) -> {
+		sort(rootElements, (r1, r2) -> {
 			if(allocationTableClassName.equals(r1.getClass().getName())) {
 				return 1;
 			} else if(allocationTableClassName.equals(r2.getClass().getName())) {
@@ -227,7 +213,7 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 				if(node != null) {
 					// Collect violations and store them in the error and warning metric
 					List<IConstraintViolation<? extends EObject>> violations = entry.getValue();
-					var integers = node.getStoredIntegers();
+					var integers = node.getIntegerMetrics();
 					integers.put(MetricKey.CONSTRAINT_VIOLATIONS_ERROR, (int)violations.stream()
 							.filter(v -> v.getSeverity() == ESeverity.ERROR).count());
 					integers.put(MetricKey.CONSTRAINT_VIOLATIONS_WARNING, (int)violations.stream()
@@ -260,10 +246,8 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 	private void recursivlyCollectMetrics(MetricTreeNode node, IHierarchicElement currentElement,
 			int recursionLevel) {
 
-		var metricData = metricDataContainer;
-
 		collectMetrics(node, currentElement);
-		node.getStoredIntegers().put(MetricKey.NESTING_LEVEL, recursionLevel);
+		node.getIntegerMetrics().put(MetricKey.NESTING_LEVEL, recursionLevel);
 
 		// Check if any of the specifications is an IHierarchicElement
 		// This is for example the case for AF3 StateAutomatons
@@ -278,7 +262,7 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 					specificationElement.getContainedElements();
 			if(containedElements.size() == 1) {
 				// Add reference from the element to this, so the lookups works as expected
-				metricData.getTreeNodeLookupTable().put(specificationElement, node);
+				metricDataContainer.getTreeNodeLookupTable().put(specificationElement, node);
 				// There is exactly one contained element. Skip the specification element to get a
 				// more useful tree structure
 				recursivlyCollectMetrics(node, containedElements.get(0), recursionLevel);
@@ -286,19 +270,19 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 				recursivlyCollectMetrics(node, specificationElement, recursionLevel);
 			}
 			// Add reference from the element to this, so the lookups works as expected
-			metricData.getTreeNodeLookupTable().put(currentElement, node);
+			metricDataContainer.getTreeNodeLookupTable().put(currentElement, node);
 		} else {
 			// Iterate over all children and merge values
-			var nodeDoubles = node.getStoredDoubles();
-			var nodeIntegers = node.getStoredIntegers();
+			var nodeDoubles = node.getDoubleMetrics();
+			var nodeIntegers = node.getIntegerMetrics();
 			for(IHierarchicElement containedElement : currentElement.getContainedElements()) {
 				MetricTreeNode child = new MetricTreeNode();
 				node.getChildren().add(child);
 				recursivlyCollectMetrics(child, containedElement, recursionLevel + 1);
 
 				for(MetricKey key : MetricKey.getCollectorKeys()) {
-					var childDoubles = child.getStoredDoubles();
-					var childIntegers = child.getStoredIntegers();
+					var childDoubles = child.getDoubleMetrics();
+					var childIntegers = child.getIntegerMetrics();
 					if(childDoubles.containsKey(key)) {
 						nodeDoubles.merge(key, childDoubles.get(key), Double::sum);
 					}
@@ -308,12 +292,14 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 				}
 			}
 
-			metricData.getTreeNodeLookupTable().put(currentElement, node);
-			
+			metricDataContainer.getTreeNodeLookupTable().put(currentElement, node);
+
 			// TODO (#4332) Restructure GraphMetricsProvider in quality Plugin
-			GraphMetricsProvider.calculateBetweennessCentrality(currentElement, metricData, false);
-			GraphMetricsProvider.calculateBetweennessCentrality(currentElement, metricData, true);
-			node.getStoredDoubles().put(MetricKey.CLUSTERING_COEFFICIENT,
+			GraphMetricsProvider.calculateBetweennessCentrality(currentElement, metricDataContainer,
+					false);
+			GraphMetricsProvider.calculateBetweennessCentrality(currentElement, metricDataContainer,
+					true);
+			node.getDoubleMetrics().put(MetricKey.CLUSTERING_COEFFICIENT,
 					GraphMetricsProvider.calculateClusteringCoefficent(currentElement));
 		}
 	}
@@ -333,7 +319,7 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 		List<IMetricProvider<? extends EObject>> providers = getAllMetricProviders(object);
 
 		for(IMetricProvider<? extends EObject> provider : providers) {
-			//TODO (#4331)
+			// TODO (#4331)
 			((IMetricProvider<EObject>)provider).collectMetrics(node, object);
 		}
 	}
@@ -416,10 +402,18 @@ import org.fortiss.tooling.kernel.utils.EcoreUtils;
 	public void topLevelElementContentChanged(ITopLevelElement element) {
 		scheduleMetricCollection(element);
 	}
-	
+
 	/** {@inheritDoc} */
 	@Override
 	public MetricData getMetricData() {
 		return metricDataContainer;
 	}
+
+	/** {@inheritDoc} */
+	@Override
+	public void createQualityFolder() {
+		if(!ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR.exists()) {
+			ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR.mkdirs();
+		}
+	}
 }

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/.ratings

-CSVFileWriter.java 89ed8b8b05dbbe55c30bd416e9255e3896ca980c YELLOW
-ModelQualityStorageManager.java c5a6783f51f596663bf836f98abf2b23130fe180 YELLOW
+CSVFileWriter.java ba986ec60099bf94472f9e80cba10c01c85bc73f GREEN
+ModelQualityStorageManager.java c5a6783f51f596663bf836f98abf2b23130fe180 GREEN

org.fortiss.tooling.ext.quality/src/org/fortiss/tooling/ext/quality/storage/CSVFileWriter.java

@@ -15,6 +15,9 @@
 +--------------------------------------------------------------------------*/
 package org.fortiss.tooling.ext.quality.storage;
 
+import static java.util.stream.Collectors.toList;
+import static org.apache.commons.io.FilenameUtils.getBaseName;
+
 import java.io.BufferedReader;
 import java.io.BufferedWriter;
 import java.io.File;
@@ -34,8 +37,7 @@ import java.util.Map;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import org.apache.commons.io.FilenameUtils;
-import org.eclipse.core.runtime.CoreException;
+import org.eclipse.core.runtime.NullProgressMonitor;
 import org.fortiss.tooling.ext.quality.data.DataRootElement;
 import org.fortiss.tooling.ext.quality.data.MetricKey;
 import org.fortiss.tooling.ext.quality.service.IModelQualityService;
@@ -49,10 +51,15 @@ import org.fortiss.tooling.kernel.model.IProjectRootElement;
  */
 public class CSVFileWriter {
 
+	/** Suffix of the file in which the data is written. */
+	private static final String CSV = ".csv";
+
 	/**
-	 * Method to write the data into CSV files. For each project one file is built. If it is already existant,
-	 * it uses to the existing keys in the file as dimensions, to help that the log file always remains consistent.
-	 * Newly added MetricKeys are  not added in the next exportations if it was not present and build.
+	 * Method to write the data into CSV files. For each project, one file is built. If it already
+	 * exists, it uses to the existing keys in the file as dimensions, to help that the log file
+	 * always remains consistent.
+	 * 
+	 * TODO (#4334) Newly added {@link MetricKey}s are not added in the next export
 	 * 
 	 * @param map
 	 *            to save into CSV file
@@ -60,31 +67,40 @@ public class CSVFileWriter {
 	 * @throws NoSuchAlgorithmException
 	 * 
 	 */
-
-	/** Suffix of the file in which the data is written. */
-	private static final String CSV = ".csv";
-	
 	public static void metricExtractionToCSV(Map<IProjectRootElement, DataRootElement> map)
-			throws NoSuchAlgorithmException, IOException, CoreException {
+			throws NoSuchAlgorithmException, IOException {
 
 		// check if there is any data to export
 		if(map == null || map.isEmpty()) {
 			return;
 		}
-		
-		IModelQualityService.createQualityFolder();
 
-		// path to CSV file
+		IModelQualityService.getInstance().createQualityFolder();
+
+		// Map is not empty due to check above
 		DataRootElement firstValue = map.entrySet().iterator().next().getValue();
 		ITopLevelElement topLevelElement = firstValue.getTopLevelElement();
 		String projectName = topLevelElement.getSaveableName();
 
-		var fileBaseName = FilenameUtils.getBaseName(projectName).toString();
-		Path path = new File(ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR, fileBaseName.concat(CSV)).toPath();
-
-		//save before using the saved file to produce the hash
-		//FIXME (KB): produces unstoppable exceptions if I use it (IllegalStateExceptions)
-		//topLevelElement.doSave(new NullProgressMonitor());
+		var fileBaseName = getBaseName(projectName).toString();
+		// TODO (#4333)
+		Path path = new File(ModelQualityStorageManager.MODEL_QUALITY_PROJECT_DIR,
+				fileBaseName.concat(CSV)).toPath();
+
+		// Metric extraction is triggered when model files are saved. Therefore, the topLevelElement
+		// should never be dirty here. In order to be on the safe side, conditionally save model.
+		// save before using the saved file to produce the hash
+		if(topLevelElement.isDirty()) {
+			try {
+				topLevelElement.doSave(new NullProgressMonitor());
+				// Avoid duplicated metric extraction (the save operation will trigger another one).
+				return;
+			} catch(Exception e) {
+				e.printStackTrace();
+			}
+			return;
+		}
+		// TODO (#4333)
 		Path modelFile = new File(ModelQualityStorageManager.MODEL_PROJECT_DIR,
 				topLevelElement.getSaveableName()).toPath();
 		String projectHash = computeGitObjectHash(modelFile);
@@ -141,7 +157,7 @@ public class CSVFileWriter {
 				// Collect all MetricKeys, convert them to a string and append them to the allKeys
 				// list
 				allKeys.addAll(Arrays.stream(MetricKey.values()).map(MetricKey::toString)
-						.map(String::toLowerCase).collect(Collectors.toList()));
+						.map(String::toLowerCase).collect(toList()));
 
 				// Write first line
 				writer.write(String.join(",", allKeys));
@@ -160,12 +176,12 @@ public class CSVFileWriter {
 				String rootName = rootElement.getClass().getSimpleName();
 				var rootNode = dataRootElement.getRootNode();
 				rootNode.traverseTree(node -> {
-					var integers = node.getStoredIntegers();
-					var doubles = node.getStoredDoubles();
-					var strings = node.getStoredStrings();
+					var integers = node.getIntegerMetrics();
+					var doubles = node.getDoubleMetrics();
+					var strings = node.getStringMetrics();
 					// collect IDs of children
 					String children_ids = node.getChildren().stream()
-							.map(n -> n.getStoredIntegers().get(MetricKey.UNIQUE_ID))
+							.map(n -> n.getIntegerMetrics().get(MetricKey.UNIQUE_ID))
 							.map(id -> id.toString()).reduce((a, b) -> a + " " + b).orElse("");
 
 					// These 6 data entries are just concatenated in front
@@ -176,8 +192,7 @@ public class CSVFileWriter {
 							.map(String::toUpperCase).map(MetricKey::valueOf).map(k -> {
 								if(k.isStringValue()) {
 									return strings.get(k);
-								}
-								if(k.isDoubleValue()) {
+								} else if(k.isDoubleValue()) {
 									return doubles.get(k);
 								}
 								return integers.get(k);
@@ -218,7 +233,8 @@ public class CSVFileWriter {
 
 			// Construct pre-image (input to hash function) according to Git specification
 			String fileContents = inputBuilder.toString();
-			// fileContexts.length() does not return the correct length on some platforms (e.g., Linux)
+			// fileContexts.length() does not return the correct length on some platforms (e.g.,
+			// Linux)
 			int n = fileContents.getBytes().length;
 			String preImage = "blob " + n + "\0" + fileContents;