Issues with CodePro Analitic

[GregTD]

So, I just installed and ran Analytics. It provided me one potentially helpful bit of information (efficiency difference when appending a single character as a character constant, rather than a String constant), but had several problems:

1: I have a number of catch statements where I do nothing. (For example: I ignore any result / problem when I close a connection to a database, since I don't do commit on close, or anything else like that.) These are all carefully labeled with a comment that says "Ignore", or "Ignore, because ...." It would be helpful for the tool to look for such a comment.

2: It told me I had an unused import, when the class I'm importing is used about 10 times.

3: If told me I needed to use instanceof in my equals routine to check if I was dealing with something of my class. Useful advice, except that the first line of the routine does that.

3a: It then went on to tell me that I shouldn't have an equals routine taking a parameter of my class. ??? You want me to have special equals code for objects of my own class, but you don't want me to put that code into a separate routine? Or you just don't want me to call that routine "equals"? Or do you want me to not make that routine "private"?

4: Then there's this gem:
The method Runtime.exec() should not be used to execute commands because the format of command execution is platform dependent.
Recommendation
1. Find a platform independent way of causing the same behavior.

Hey, if you find one, let me know. However, since I have to launch this other (C++) program, until you can actually offer a platform independent way to do that, that's a pretty useless bit of feedback, no?

4: This one is also pretty special:
The while statement is empty.
Recommendation
1. Remove the while statement or fix the code so that its body is no longer empty.

In the particular place it's complaining about:
a: I have a comment in the while body, pointing out that all the execution is done in the part (this keeps the java compiler from warning me about having an empty statement)
b: I'm skipping blank lines in a text file. While means I have to read a line, make sure I got a line, and then see if the line is empty. I suppose I could put the second test into the body of the while, along with a "break" command, but I fail to see how that is "fixing" the code (at least, I fail to see how it is fixing the code in a positive manner).

5: "Caught exception not logged." No, it's not. That's because I'm checking users entered parameters, and reporting to them if their entry caused a NumberFormatException. Not being a fault in my code, there's nothing to log. Or I tried something one way, and it didn't work, so I tried it another way. Or I'm closing a SQL Statement, and couldn't care less if that generates a problem.

So, again, if there's a comment saying "ignore", or a call to System.err.println, the odds are high that you shouldn't be reporting that error.

Because if 90%+ of the "errors" the user gets are useless, you're going to get ignored.

[Brian Wilkerson]

1: I have a number of catch statements where I do nothing. (For example: I ignore any result / problem when I close a connection to a database, since I don't do commit on close, or anything else like that.) These are all carefully labeled with a comment that says "Ignore", or "Ignore, because ...." It would be helpful for the tool to look for such a comment.

It can. If you open the Eclipse Preferences dialog to the CodePro > Audit page, select the Rules tab, and then expand Possible Errors and select the Empty Catch Clause rule, in the Parameters tab below the rule tree you'll find a check box that allows you to specify that the rule should treat catch clauses with comments as if they were not empty. Selecting that option should make the rule behave the way you want it to.

2: It told me I had an unused import, when the class I'm importing is used about 10 times.

That sounds like a bug. Could you provide us with a simple test case so that we can reproduce and fix it? Thanks.

3: If told me I needed to use instanceof in my equals routine to check if I was dealing with something of my class. Useful advice, except that the first line of the routine does that.

That might also be a bug. It sounds like the rule isn't recognizing your use of instanceof for some reason. Again, could you send us a simple test case that reproduces the problem? Thanks.

3a: It then went on to tell me that I shouldn't have an equals routine taking a parameter of my class. ??? You want me to have special equals code for objects of my own class, but you don't want me to put that code into a separate routine? Or you just don't want me to call that routine "equals"? Or do you want me to not make that routine "private"?

If you're using the Overloaded Methods rule, the problem that it's trying to help you avoid is inconsistent semantics caused by overloaded methods in which two or more methods could potentially be selected depending on how much information the compiler has. If the overloaded methods both return the same result when passed the same object, then there is no problem. Unfortunately, we can't ensure that that's the case through static analysis.

If a given rule does not meet your needs, you should check to see whether the rule can be configured differently. Many of our rules can. This rule, for example, has a check box that allows you to configure it to ignore cases where both the generic and specific methods are implemented by the same class (the problem is much more likely to occur if the implementor of a class might not be aware that they are inheriting a similar method from a superclass). That might take care of the false positives you're seeing.

4: Then there's this gem:
The method Runtime.exec() should not be used to execute commands because the format of command execution is platform dependent.

However, since I have to launch this other (C++) program, until you can actually offer a platform independent way to do that, that's a pretty useless bit of feedback, no?

If a rule cannot be configured to meet your needs, please let us know what we can do to make it more useful.

Sometimes, and this seems to be one of those cases, there just isn't anything you can do other than violating the rule. When that's the case you have two options. One is to globally disable the rule by deselecting it in the rule tree on the CodePro > Audit preference page. The other is to locally disable the rule in the one place where you really need an exception to the rule. You can get directions for doing so here: http://code.google.com/webtoolkit/tools/codepro/doc/features/audit/locally_disabling_audit_rules.html

4: This one is also pretty special:
The while statement is empty.

In the particular place it's complaining about:
a: I have a comment in the while body, pointing out that all the execution is done in the part (this keeps the java compiler from warning me about having an empty statement)
b: I'm skipping blank lines in a text file. While means I have to read a line, make sure I got a line, and then see if the line is empty. I suppose I could put the second test into the body of the while, along with a "break" command, but I fail to see how that is "fixing" the code (at least, I fail to see how it is fixing the code in a positive manner).

This is another good example of a place where a useful rule is sometimes wrong. Generally speaking, empty while loops are a problem because the conditions usually don't have a side-effect. In cases where they do, I'd agree that you generally don't want to force the code into this pattern. Again, you can either disable the rule globally, or disable it for this one case and let it help you find other potential mistakes.

5: "Caught exception not logged." No, it's not. That's because I'm checking users entered parameters, and reporting to them if their entry caused a NumberFormatException. Not being a fault in my code, there's nothing to log. Or I tried something one way, and it didn't work, so I tried it another way. Or I'm closing a SQL Statement, and couldn't care less if that generates a problem.

So, again, if there's a comment saying "ignore", or a call to System.err.println, the odds are high that you shouldn't be reporting that error.

We'll look into the possibility of adding an option to ignore more of these cases. We would certainly prefer to have options that can be configured to make the rules work as expected (without false positives) than to require users to locally disable rules in lots of places. We are also working to review our rules to see whether the default configuration can be improved to work for a wider range of users.

Because if 90%+ of the "errors" the user gets are useless, you're going to get ignored.

Absolutely!

That's why the tool has so many configuration options. The downside of all those configuration parameters is that it takes some time to get it configured in a way that works with your coding style and practices. You need to review all of the rules (and there are a lot of them) to decide which ones you care about and which you don't, and then you have to configure many of them to work the way you want them to. Simplifying this process is one of our top priorities.

[Eric Clayberg]

4: Then there's this gem:
The method Runtime.exec() should not be used to execute commands because the format of command execution is platform dependent.
Recommendation
1. Find a platform independent way of causing the same behavior.
Hey, if you find one, let me know. However, since I have to launch this other (C++) program, until you can actually offer a platform independent way to do that, that's a pretty useless bit of feedback, no?

Actually, this is a very important piece of feedback depending on who is reviewing the code and what the code actually does. Java is meant to be portable across platforms. Knowing that your app contains code that is not portable is a useful thing to know. Whether you can actually do anything about it is a different issue entirely, so you may want to either turn that rule off or tell the system to ignore that specific instance. There can also certainly be cases where a call to Runtime.exec() on some OS code can be replace by a call to some built in Java function, but the tool has no way of knowing that. All it can do is identify that you have an issue that you need to be aware of (and can choose to ignore).