OK, after reviewing some code with PMD and FindBugs code analyzers, i was able to do great changes on the reviewed code. However, there are some things i don't know how to fix. I'll iterate them bellow, and (for better reference) i will give each question a number. Feel free to answer to any/all of them. Thanks for your patience.
1. Even tough i have removed some of the rules, the associated warnings are still there, after re-evaluate the code. Any idea why?
2. Please look at the declarations :
private Combo comboAdress;
private ProgressBar pBar;
and the references to objects by getters and setters :
private final Combo getComboAdress() {
return this.comboAdress;
}
private final void setComboAdress(final Combo comboAdress) {
this.comboAdress = comboAdress;
}
private final ProgressBar getpBar() {
return this.pBar;
}
private final void setpBar(final ProgressBar pBar) {
this.pBar = pBar;
}
Now, i wonder why the first declaration don't give me any warning on PMD, while the second gives me the following warning :
Found non-transient, non-static member. Please mark as transient or provide accessors.
More details on that warning here.
3. Here is another warning, also given by PMD :
A method should have only one exit point, and that should be the last statement in the method
More details on that warning here.
Now, i agree with that, but what if i write something like this :
public void actionPerformedOnModifyComboLocations() {
if (getMainTree().isFocusControl()) {
return;
}
....//do stuffs, based on the initial test
}
I tend to agree with the rule, but if performance of the code suggest multiple exit points, what should i do?
4. PMD gives me this :
Found 'DD'-anomaly for variable 'start_page' (lines '319'-'322').
when i declare something like :
String start_page = null;
I get rid of this info (level of warning is info) if i remove the assignment to null, but..i got an error from IDE, saying that the variable could be uninitialized, at some point later in the code. So, i am kind of stuck with that. Supressing the warning is the best you can do?
5. PMD Warning :
Assigning an Object to null is a code smell. Consider refactoring.
This is the case of a singletone use of GUI components or the case of a method who returns complex objects. Assigning the result to null in the catch() section it's justified by the need to avoid the return of an incomplete/inconsistent object. Yes, NullObject should be used, but there are cases where i don't want to do that. Should i supress that warning then?
6. FindBugs warning #1:
Write to static field MyClass.instance from instance method MyClass.handleEvent(Event)
in the method
@Override
public void handleEvent(Event e) {
switch (e.type) {
case SWT.Dispose: {
if (e.widget == getComposite()) {
MyClass.instance = null;
}
break;
}
}
}
of the static variable
private static MyClass instance = null;
The variable allows me to test whether the form is already created and visible or not, and i need to force the re-creation of the form, in some cases. I see no other alternative here. Any insights? (MyClass implements Listener, hence the overrided handleEvent() method).
7. FindBugs warning #2:
Class MyClass2 has a circular dependency with other classes
This warning is displayed based on simple imports of other classes. Do i need to refactor those imports to make this warning go away? Or the problem relies in MyClass2?
OK, enough said for now..expect an update, based on more findings and/or your answers. Thanks.
Here are my answers to some of your questions:
Question number 2:
I think you're not capitalizing the properties properly. The methods should be called getPBar and setPBar.
String pBar;
void setPBar(String str) {...}
String getPBar() { return pBar};
The JavaBeans specification states that:
For readable properties there will be a getter method to read the property value. For writable properties there will be a setter method to allow the property value to be updated. [...] Constructs a PropertyDescriptor for a property that follows the standard Java convention by having getFoo and setFoo accessor methods. Thus if the argument name is "fred", it will assume that the reader method is "getFred" and the writer method is "setFred". Note that the property name should start with a lower case character, which will be capitalized in the method names.
Question number 3:
I agree with the suggestion of the software you're using. For readability, only one exit point is better. For efficiency, using 'return;' might be better. My guess is that the compiler is smart enough to always pick the efficient alternative and I'll bet that the bytecode would be the same in both cases.
FURTHER EMPIRICAL INFORMATION
I did some tests and found out that the java compiler I'm using (javac 1.5.0_19 on Mac OS X 10.4) is not applying the optimization I expected.
I used the following class to test:
public abstract class Test{
public int singleReturn(){
int ret = 0;
if (cond1())
ret = 1;
else if (cond2())
ret = 2;
else if (cond3())
ret = 3;
return ret;
}
public int multReturn(){
if (cond1()) return 1;
else if (cond2()) return 2;
else if (cond3()) return 3;
else return 0;
}
protected abstract boolean cond1();
protected abstract boolean cond2();
protected abstract boolean cond3();
}
Then, I analyzed the bytecode and found that for multReturn() there are several 'ireturn' statements, while there is only one for singleReturn(). Moreover, the bytecode of singleReturn() also includes several goto to the return statement.
I tested both methods with very simple implementations of cond1, cond2 and cond3. I made sure that the three conditions where equally provable. I found out a consistent difference in time of 3% to 6%, in favor of multReturn(). In this case, since the operations are very simple, the impact of the multiple return is quite noticeable.
Then I tested both methods using a more complicated implementation of cond1, cond2 and cond3, in order to make the impact of the different return less evident. I was shocked by the result! Now multReturn() is consistently slower than singleReturn() (between 2% and 3%). I don't know what is causing this difference because the rest of the code should be equal.
I think these unexpected results are caused by the JIT compiler of the JVM.
Anyway, I stand by my initial intuition: the compiler (or the JIT) can optimize these kind of things and this frees the developer to focus on writing code that is easily readable and maintainable.
Question number 6:
You could call a class method from your instance method and leave that static method alter the class variable.
Then, your code look similar to the following:
public static void clearInstance() {
instance = null;
}
@Override
public void handleEvent(Event e) {
switch (e.type) {
case SWT.Dispose: {
if (e.widget == getComposite()) {
MyClass.clearInstance();
}
break;
}
}
}
This would cause the warning you described in 5, but there has to be some compromise, and in this case it's just a smell, not an error.
Question number 7:
This is simply a smell of a possible problem. It's not necessarily bad or wrong, and you cannot be sure just by using this tool.
If you've got a real problem, like dependencies between constructors, testing should show it.
A different, but related, problem are circular dependencies between jars: while classes with circular dependencies can be compiled, circular dependencies between jars cannot be handled in the JVM because of the way class loaders work.