added ccmCalculus especially to find ncc value for ccm metric. The lo… #548

starkda · 2024-03-20T18:06:52Z

…gic that finds connected components not implemented yet.
one of pull requests that resolve #522

In this pr I've added separate calculus that would be used to find CCM metrics. It would traverse over existing XML skeleton, and add . NCC it is not calculated yet, it is always 1. In further prs I would add logic for it and sligthly modify CCM.xsl to capture ncc from skeleton.

…gic that finds connected components not implemented yet.

starkda · 2024-03-20T18:07:28Z

@yegor256 please review

yegor256 · 2024-03-20T18:13:19Z

@pnatashap can you please take a look?

pnatashap · 2024-03-20T18:32:28Z

src/test/java/org/jpeek/calculus/java/CcmCalculusTest.java

+            "Must have 2 ncc vars",
+            result.xpath("/metric/app/package/class/vars/var[@id=\'ncc\']/text()").size() == 2,
+            new IsTrue()
+        );


Suggested change

);

).affirm();

pnatashap · 2024-03-20T18:56:38Z

src/main/java/org/jpeek/calculus/xsl/CcmXslCalculus.java

+ *
+ * @since 0.30.9
+ */
+public final class CcmXslCalculus implements Calculus {


There is an existing class https://github.com/cqfn/jpeek/blob/master/src/main/java/org/jpeek/calculus/java/Ccm.java for this metric. I do not see any reason for the new class.

pnatashap · 2024-03-20T18:58:30Z

src/main/java/org/jpeek/calculus/xsl/CcmXslCalculus.java

+            new UncheckedText(
+                new TextOf(
+                    new ResourceOf(
+                        new FormattedText("org/jpeek/metrics/%s.xsl", metric)


Please take a look in referenced class implementation, only CMM metric should be allowed

pnatashap · 2024-03-20T19:04:44Z

@starkda as implementation is not completed, all things that are not completed should be in @todo puzzle in the code, not in PR. Pull request will be closed and all future plans from it will not be accessible for anybody, but their must be.

starkda · 2024-03-21T11:57:23Z

@pnatashap please review

pnatashap · 2024-03-22T01:52:38Z

@starkda looks like some loop is possible, test for macos with jdk 11 failed due time-out (6 hours)

pnatashap · 2024-03-22T05:08:08Z

@starkda It would be better if you create all required tests with manually calculated metrics (ccm and all parts inside) to understands what we want to achieve. (should be with one connectivity component inside, with several of them, with and without constructor inside). This will really add some value for the next steps.
And I do not see any huge problem to calculate it inside of xsl: all information about methods and attribute usage, so using call-template with parameters looks like possible to divide it into the groups. Even if we can not calculate something in xsl, we can calculate it and pass as a parameter to XSL, because reading all this xml document transformation is really very difficult.

starkda · 2024-03-22T14:37:49Z

@starkda looks like some loop is possible, test for macos with jdk 11 failed due time-out (6 hours)

@pnatashap I guess it is not problem from the changes i made. I effectively changed nothing, but now it works

starkda · 2024-03-22T15:01:08Z

@starkda It would be better if you create all required tests with manually calculated metrics (ccm and all parts inside) to understands what we want to achieve. (should be with one connectivity component inside, with several of them, with and without constructor inside). This will really add some value for the next steps. And I do not see any huge problem to calculate it inside of xsl: all information about methods and attribute usage, so using call-template with parameters looks like possible to divide it into the groups. Even if we can not calculate something in xsl, we can calculate it and pass as a parameter to XSL, because reading all this xml document transformation is really very difficult.

@pnatashap,

You're right. I'll add more test in further pull request(I've added puzzle in code for more tests)
That's what i did: we cannot calculate number of connected components(ncc) in xsl so I passed it as a parameter. Yet it looks terrifying I guess that's the best option, I wish I could delegate some steps to xsl, but the problem in very last step: running dfs.

pnatashap

As we have am example with invalid metric calculation, so it is better to create a tests (in disabled status) to highlight all situations that we need to cover. After that we can start working on fixing

pnatashap · 2024-03-22T15:10:59Z

src/main/java/org/jpeek/calculus/java/Ccm.java

@@ -68,6 +84,7 @@ public XML node(
     * @param skeleton XML Skeleton
     * @return XML with fixed NCC.
     */
+    @SuppressWarnings("PMD.UnusedPrivateMethod")


really bed suppression, method is not used and confusing for all, why do we need it

pnatashap · 2024-03-22T15:14:55Z

src/main/java/org/jpeek/calculus/java/Ccm.java

+                    )
+                )
+            ).asString(),
+            Sources.DUMMY,


not clear why do we need to apply xsl for dymmy sources, while ccm contains 5 columns for now and we have a problem with 2 of them only
https://user-images.githubusercontent.com/63350301/117303386-4ff0da00-ae85-11eb-82c3-869f1a66521d.png

pnatashap · 2024-03-22T15:18:09Z

src/main/java/org/jpeek/calculus/java/Ccm.java

+            builder = factory.newDocumentBuilder();
+            doc = builder.newDocument();
+            final Element meta = doc.createElement("meta");
+            Node packages = skeleton.node().getFirstChild().getFirstChild().getNextSibling()


Really near to impossible to support this code, I can not remember the order of the nodes in XML and nobody wants, that's why there are a lot of ways to work with XML using XSL, XPATH, tag names.
If you walk with order only, your code will be broken by any sceleton structure changes.

pnatashap · 2024-03-22T15:19:14Z

src/main/java/org/jpeek/calculus/java/Ccm.java

+        final Node id = map.getNamedItem("id");
+        root.setAttribute("id", id.getNodeValue());
+        final Integer value = 1;
+        final Element ncc = doc.createElement("ncc");


not only ncc metric should be presented, you can check current implementation
https://user-images.githubusercontent.com/63350301/117303386-4ff0da00-ae85-11eb-82c3-869f1a66521d.png

pnatashap · 2024-03-22T15:20:41Z

src/test/java/org/jpeek/calculus/java/CcmTest.java

+        ).affirm();
+        new Assertion<>(
+            "Must have 2 ncc vars",
+            result.xpath("/metric/app/package/class/vars/var[@id=\'ncc\']/text()").size() == 2,


While we have a specific problem with specific class, lets create a tests to check metric values, not just their existing.

pnatashap · 2024-03-22T15:55:54Z

@starkda you have already one example for the test in issue, it is very easy to add a test for it. all other code does not bring anything usefull in the project, just brake existing even more

pnatashap · 2024-03-23T14:29:18Z

@starkda I've added a tests and find an issue with nc calculation. It is definitely can be solved in xsl (and the issue with build freezing is also solved)

starkda · 2024-03-23T15:49:24Z

@pnatashap could you please elaborate how it can be made in xsl. Any approach requires to change state while xsl variables can not be changed after initialization

pnatashap · 2024-03-23T16:24:44Z

@starkda for fix nc calculation you just need to add one more condition in if and that it.
variables in xsl can not be changed, but you can call template with any value that you need. I still agree with this comment #523 (comment)

…gic that finds connected components not implemented yet.

pnatashap · 2024-03-26T02:55:02Z

src/main/resources/org/jpeek/metrics/CCM.xsl

+      <xsl:for-each select="$methods">
+        <xsl:variable name="method" select="."/>
+        <node id="{$method/@name}">
+          <xsl:for-each select="$edges/edge[method[1]/text() = $method/@name]">


Suggested change

<xsl:for-each select="$edges/edge[method[1]/text() = $method/@name]">

<xsl:for-each select="$edges/edge[method/text() = $method/@name]">

pnatashap · 2024-03-26T02:55:52Z

src/main/resources/org/jpeek/metrics/CCM.xsl

+        <node id="{$method/@name}">
+          <xsl:for-each select="$edges/edge[method[1]/text() = $method/@name]">
+            <edge>
+              <xsl:value-of select="method[2]/text()"/>


Suggested change

<xsl:value-of select="method[2]/text()"/>

<xsl:value-of select="method[text() != $method/@name]/text()"/>

pnatashap · 2024-03-26T02:56:42Z

src/main/resources/org/jpeek/metrics/CCM.xsl

+          <xsl:for-each select="$edges/edge[method[1]/text() = $method/@name]">
+            <edge>
+              <xsl:value-of select="method[2]/text()"/>
+            </edge>


If apply two changes below, then you do not need to duplicate edges

starkda · 2024-03-29T15:09:25Z

@pnatashap what about returning to my initial approach and use java instead of xsl to calculate ncc? I suppose the main problem with heap overflow can not be solved anyway. Such solution eats a lot of memory and may not be acceptable for more or less big repos

pnatashap · 2024-03-29T16:48:23Z

@starkda you can try to calculate it, not just add a comment, that it should be calculated here, as we have already a task to calculate it properly

starkda · 2024-04-01T19:40:05Z

@pnatashap please review.
hope it more readeable now.
To find components i used union find instead of dfs

src/main/java/org/jpeek/calculus/java/Ccm.java

pnatashap · 2024-04-02T03:55:29Z

src/main/java/org/jpeek/calculus/java/Ccm.java

+        try {
+            final Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder()
+                .newDocument();
+            final Element meta = doc.createElement("meta");


it is not required to change XML document and add some data, it is easier to call XSL with parameter, so you need to read ncc value in XSL and use it.

You do not understand, we can run XSL for class skeleton with parameter cmm, so we do not need to create package and all others inside, as all this arledy implemented and can be changed in case if it is needed in xsl using common way. it is absolutely not readable in code, xsl is much clear

src/main/java/org/jpeek/calculus/java/Ccm.java

starkda · 2024-04-02T21:01:47Z

@pnatashap please review

pnatashap · 2024-04-03T00:04:37Z

src/main/java/org/jpeek/calculus/java/Ccm.java

+        try {
+            final Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder()
+                .newDocument();
+            final Element meta = doc.createElement("meta");


You do not understand, we can run XSL for class skeleton with parameter cmm, so we do not need to create package and all others inside, as all this arledy implemented and can be changed in case if it is needed in xsl using common way. it is absolutely not readable in code, xsl is much clear

src/main/java/org/jpeek/calculus/java/Ccm.java

starkda · 2024-04-04T13:24:39Z

@pnatashap please review
I've put Union-Find into separate class. I've also created separate package for it, as i did not find suitable place for it.
Graph package is not a good place for it i think, as it is related to xml.

pnatashap

Still not clear to support

pnatashap · 2024-04-07T00:26:53Z

src/main/java/org/jpeek/calculus/java/Ccm.java

-                clazz.toString()
-            ).toString()
+        final Element ncc = doc.createElement("ncc");
+        ncc.appendChild(doc.createTextNode(calculateComponents(clazz, params).toString()));


Now incapsulation is better, but according to OOP we should have
ncc.appendChild(doc.createTextNode(new GraphClass(clazz, params).components().lenght().toString()));
and all other calculations should be inside of that class

pnatashap · 2024-04-07T01:07:06Z

src/main/java/org/jpeek/calculus/java/Ccm.java

-                Ccm.updateNcc(skeleton, pack, clazz);
+    private static XML addMetaInformation(final XML skeleton, final Map<String, Object> params) {
+        try {
+            final Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder()


I do not understand why you ignore the comment below. This part of the code is very difficult to support. You can add child element to the class without coping all this structure.

pnatashap · 2024-04-07T01:12:15Z

src/main/resources/org/jpeek/metrics/CCM.xsl

@@ -56,13 +61,21 @@ SOFTWARE.
                <xsl:value-of select="$other/@name"/>
              </method>
            </edge>
+            <edge>


this should be reverted as I understand

starkda · 2024-04-18T16:11:44Z

@pnatashap I re-implemented CcmCalculus using XmlGraph. I also added several xsl files to add meta information without in-code xml modification.
XmlGraph implementation has some bugs, so it does not always give correct ncc.

added ccmCalculus especially to find ncc value for ccm metric. The lo…

aebacaa

…gic that finds connected components not implemented yet.

pnatashap suggested changes Mar 20, 2024

View reviewed changes

starkda added 3 commits March 21, 2024 14:38

moved cmm stuff into ccm class

c9293ab

moved cmm stuff into ccm class

fd730b6

moved cmm stuff into ccm class

4443ead

moved cmm stuff into ccm class

e0f8a2f

starkda requested a review from pnatashap March 21, 2024 22:07

moved cmm stuff into ccm class

0ce54fd

pnatashap suggested changes Mar 22, 2024

View reviewed changes

added todos on tests

aaa584a

starkda added 8 commits March 25, 2024 13:25

added ccmCalculus especially to find ncc value for ccm metric. The lo…

9557ee8

…gic that finds connected components not implemented yet.

moved cmm stuff into ccm class

3b47211

moved cmm stuff into ccm class

144e4e1

moved cmm stuff into ccm class

1f7f100

moved cmm stuff into ccm class

dd92650

moved cmm stuff into ccm class

61dbe52

added todos on tests

cdc7ae4

Merge remote-tracking branch 'origin/ccmCalculus' into ccmCalculus

5017e4a

pnatashap suggested changes Mar 26, 2024

View reviewed changes

starkda added 3 commits April 1, 2024 21:21

ncc calculation using java

47254cc

fixed tests

a8684bd

fixed tests

f6489cb

pnatashap suggested changes Apr 2, 2024

View reviewed changes

starkda added 8 commits April 2, 2024 20:23

fixed tests

93668c3

fixes

4f56dfb

license added

762b62f

fixes

ddce255

fixes

b4cf48b

fixes

debef9d

fixes

39b8f2f

fixes

da5370e

pnatashap suggested changes Apr 3, 2024

View reviewed changes

starkda added 2 commits April 4, 2024 14:18

fixes

69c8980

fixes

202e99e

starkda requested a review from pnatashap April 6, 2024 15:38

pnatashap suggested changes Apr 7, 2024

View reviewed changes

starkda added 3 commits April 17, 2024 20:14

fixes

bb8e09f

license

ec8dd38

fix

b6404bc

starkda requested a review from pnatashap April 18, 2024 15:57

fix

4b0e358

fix

52c404c

	<xsl:for-each select="$edges/edge[method[1]/text() = $method/@name]">
	<xsl:for-each select="$edges/edge[method/text() = $method/@name]">

	<xsl:value-of select="method[2]/text()"/>
	<xsl:value-of select="method[text() != $method/@name]/text()"/>

added ccmCalculus especially to find ncc value for ccm metric. The lo… #548

Are you sure you want to change the base?

added ccmCalculus especially to find ncc value for ccm metric. The lo… #548

Conversation

starkda commented Mar 20, 2024

starkda commented Mar 20, 2024

yegor256 commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnatashap commented Mar 20, 2024

starkda commented Mar 21, 2024 • edited

pnatashap commented Mar 22, 2024

pnatashap commented Mar 22, 2024

starkda commented Mar 22, 2024

starkda commented Mar 22, 2024

pnatashap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnatashap commented Mar 22, 2024

pnatashap commented Mar 23, 2024 • edited

starkda commented Mar 23, 2024

pnatashap commented Mar 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkda commented Mar 29, 2024

pnatashap commented Mar 29, 2024

starkda commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkda commented Apr 2, 2024

Choose a reason for hiding this comment

starkda commented Apr 4, 2024

pnatashap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkda commented Apr 18, 2024

starkda commented Mar 21, 2024 •

edited

pnatashap commented Mar 23, 2024 •

edited