phpmetrics / PhpMetrics

Beautiful and understandable static analysis tool for PHP
https://phpmetrics.github.io/website/
MIT License
2.48k stars 259 forks source link

SystemComplexityVisitor for methods with the same name on different objects #497

Closed andrii-pukhalevych closed 1 year ago

andrii-pukhalevych commented 1 year ago

(new B)->bar() and (new C)->bar() should be calculated separately.

niconoe- commented 1 year ago

Thank you for your contribution. However, I'm unsure what kind of issue are you solving here. Could you please elaborate and give an example?

Thanks again.

andrii-pukhalevych commented 1 year ago

https://github.com/phpmetrics/PhpMetrics/blob/4b77140a11452e63c7a9b98e0648320bf6710090/src/Hal/Metric/Class_/Structural/SystemComplexityVisitor.php#L58C1-L65C1

In current version, (new B)->bar() and (new C)->bar() counted as the same method. Because getNameOfNode($node) returns 'B' for both calls, and after array_unique, $fanout = 1. Since its 2 different methods, it should be counted as 2 separate methods.

andrii-pukhalevych commented 1 year ago

Example:

class A {

    public function foo()
    {
        (new C)->bar();
        (new B)->bar();
    }
}

class B {
    public function bar()
    {

    }
}

class C {
    public function bar()
    {

    }
}
niconoe- commented 1 year ago

Ok, fine, I got it now! Thanks a lot. I'll integrate this into the new rc version.

niconoe- commented 1 year ago

@andrii-pukhalevych Regarding current implementation reviewed on the 3.x-dev branch, it looks like everything is fine. See: https://github.com/phpmetrics/PhpMetrics/blob/77c3c6dc2544066bd42b9ecff94c036d473d994f/src/Hal/Metric/Class_/Structural/SystemComplexityVisitor.php#L72-L91

Do you agree with this implementation?

I won't support v2 anymore so if you think the calculation is fixed on v3, I'll just close this PR, otherwise, please let me know how do you think it's still bugged.

Thanks a lot.

andrii-pukhalevych commented 1 year ago

Agree, for this test file I updated, 3.x-dev branch results are almost the same as results from PR I did.

For class A: andrii-pukhalevych:patch1 : {DataComplexity: 0.5, StructuralComplexity: 36.0, SystemComplexity: 36.5] 3.x-dev: {DataComplexity: 0.25, StructuralComplexity: 40.5, SystemComplexity: 40.75}

Not sure what is correct, but the difference is quite small.

Anyway, please merge this PR into version 2, because many PHP 7 users still will use it few years :) Or I can close it, if it is not possible to make version 2 release.

niconoe- commented 1 year ago

Agree, for this test file I updated, 3.x-dev branch results are almost the same as results from PR I did.

For class A: andrii-pukhalevych:patch1 : {DataComplexity: 0.5, StructuralComplexity: 36.0, SystemComplexity: 36.5] 3.x-dev: {DataComplexity: 0.25, StructuralComplexity: 40.5, SystemComplexity: 40.75}

Not sure what is correct, but the difference is quite small.

The 3.x-dev branch comes with more elements to analyse and compare, so it might explain the difference. In any kind, you're right, the differences are small enough agree this is fixed on v3

Anyway, please merge this PR into version 2, because many PHP 7 users still will use it few years :) Or I can close it, if it is not possible to make version 2 release.

I'm sorry but even if I merge this PR in v2, I wouldn't be able to release a patch version. The releasing system has been reviewed in v3 in order to make it possible for other people than Jean-François to release, but on v2 that's not the case, unfortunately. That's the main reason I won't support PhpMetrics v2 anymore.

andrii-pukhalevych commented 1 year ago

I would like this PR to be merged in v2, even without release. Maybe it will help somebody. If you dont want, then just close, NP :)

niconoe- commented 1 year ago

Ok then :slightly_smiling_face: