Simple, general-interest, code-analyzers based, Java questions

Question

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.

Answer 1

1. I have no idea. It seems likely that whatever you did do, it was not what you were attempting to do!

2. Perhaps the declarations appear in a Serializable class but that the type (e.g. ComboProgress are not themselves serializable). If this is UI code, then that seems very likely. I would merely comment the class to indicate that it should not be serialized.

3. This is a valid warning. You can refactor your code thus:

   public void actionPerformedOnModifyComboLocations() {
       if (!getMainTree().isFocusControl()) {
           ....//do stuffs, based on the initial test
       }
   }
   

4. This is why I can't stand static analysis tools. A null assignment obviously leaves you open to NullPointerExceptions later. However, there are plenty of places where this is simply unavoidable (e.g. using try catch finally to do resource cleanup using a Closeable)

5. This also seems like a valid warning and your use of static access would probably be considered a code smell by most developers. Consider refactoring via using dependency-injection to inject the resource-tracker into the classes where you use the static at the moment.

6. If your class has unused imports then these should be removed. This might make the warnings disappear. On the other hand, if the imports are required, you may have a genuine circular dependency, which is something like this:

   class A {
       private B b;
   }
   class B {
       private A a;
   }
   

This is usually a confusing state of affairs and leaves you open to an initialization problem. For example, you may accidentally add some code in the initialization of A that requires its B instance to be initialized. If you add similar code into B, then the circular dependency would mean that your code was actually broken (i.e. you couldn't construct either an A or a B.

Again an illustration of why I really don't like static analysis tools - they usually just provide you with a bunch of false positives. The circular-dependent code may work perfectly well and be extremely well-documented.

Answer 2

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.

Answer 3

For point 3, probably the majority of developers these days would say the single-return rule is simply flat wrong, and on average leads to worse code. Others see that it a written-down rule, with historical credentials, some code that breaks it is hard to read, and so not following it is simply wrong.

You seem to agree with the first camp, but lack the confidence to tell the tool to turn off that rule.

The thing to remember is it is an easy rule to code in any checking tool, and some people do want it. So it is pretty much always implemented by them.

Whereas few (if any) enforce the more subjective 'guard; body; return calculation;' pattern that generally produces the easiest-to-read and simplest code.

So if you are looking at producing good code, rather than simply avoiding the worst code, that is one rule you probably do want to turn off.