Conversation
| if (val < 0 || val > 1) { | ||
| throw new IllegalArgumentException("Expected a number between 0 and 1"); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think, you could use String.format(...) here:
return String.format("%." + numDecimals + "f%%", 100 * val);
I think, this is without the truncate-to-Int functionality.
There was a problem hiding this comment.
That's nice indeed! We'll have to override the locale though, the decimal separator is locale-specific (for me it's a comma). Tests that pass on my machine could fail on other machines.
Truncate to int is only really needed for the tests, I think it actually looks neater/ more consistent with decimals
|
|
||
| if (methodCount > 1) { | ||
| for (int i = 0; i < methodCount; i++) { | ||
| for (int j = i + 1; j < methodCount; j++) { |
There was a problem hiding this comment.
That would compare in the last round the same method - i == j.
I think the condition for the out loop over i should be "i < methodCount - 1"?
There was a problem hiding this comment.
Hum no, in the last round, i = methodCount - 1, so j = i + 1 = methodCount, thus j < methodCount doesn't hold and the inner for loop is skipped. You wrote this ;)
Changing the outer condition might be clearer though, will do
|
|
||
|
|
||
| private String getNameImage(ASTPrimaryExpression node) { | ||
| ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); |
There was a problem hiding this comment.
Could there be a case, where there is no PrimaryPrefix? Do we need to check for null?
There was a problem hiding this comment.
I've never seen that case, not sure if that's possible... Maybe @jsotuyod would know?
There was a problem hiding this comment.
No, under the current grammar there is always a prefix, although there may be no suffix: https://github.com/pmd/pmd/blob/master/pmd-java/etc/grammar/Java.jjt#L1830-L1834
|
|
||
|
|
||
| private String getMethodOrAttributeName(ASTPrimaryExpression node) { | ||
| ASTPrimaryPrefix prefix = node.getFirstDescendantOfType(ASTPrimaryPrefix.class); |
There was a problem hiding this comment.
Could there be a case, where there is no PrimaryPrefix? Do we need to check for null?
| double totalMethodPairs = n * (n + 1) / 2.0; | ||
| return totalMethodPairs; | ||
| } | ||
| private static final int FEW_ATFD_THRESHOLD = 6; |
There was a problem hiding this comment.
FEW was before 5. Any reason why you changed it to 6?
There was a problem hiding this comment.
Err no valid reason. I guess it's not uncommon to use getters or setters of the objects you manipulate, and a total of 5 in a class seemed a bit low (to me). I don't know if it makes a difference really, I can put back 5 so as not to change the behaviour too much
| public Object visit(ASTClassOrInterfaceDeclaration node, Object data) { | ||
| int wmc = (int) JavaMetrics.get(JavaClassMetricKey.WMC, node); | ||
| double tcc = JavaMetrics.get(JavaClassMetricKey.TCC, node); | ||
| int atfd = (int) JavaMetrics.get(JavaClassMetricKey.ATFD, node); |
| language="java" | ||
| since="5.0" | ||
| message="Possible God class" | ||
| message="Possible God Class (WMC={0}, ATFD={2}, TCC={1})" |
There was a problem hiding this comment.
Nice idea to ouput the metric values
| <code-ref id="full-example"/> | ||
| </test-code> | ||
|
|
||
| <test-code> |
There was a problem hiding this comment.
you can add the attribute regressionTest="true", then this test case will be ignored.... You can then expect the correct message.
655b0b6 to
f553066
Compare
This is the new god class rule
The metrics can be improved individually later, especially ATFD, which has some known limitations (I'll have no time to do that in a few days)