15

I have the following setup which is giving me a message stating that "Constructor Calls Overridable Method". I know this is happening, but my question is how to fix it so that the code still works and the message goes away.

public interface Foo{
   void doFoo();
}
public class FooImpl implements Foo{
 @Override{
 public void doFoo(){
    //.. Do important code
 }
}
public class Bar{
  private FooImpl fi;
  public Bar(){
    fi = new FooImpl();
    fi.doFoo(); // The message complains about this line
  }
}

Thanks!

user973479
  • 1,629
  • 5
  • 26
  • 48
  • The code you've shown is calling the overridable method *after* it's called the constructor, not *from* the constructor, right? Or am I missing something? – NPE Apr 26 '12 at 15:58
  • Showing code that actually inhibits the warning would probably be a great idea.. – Voo Apr 26 '12 at 15:58
  • aix- It is raising the message because Bar is calling fi.doFoo() inside Bar's constructor. Voo - The code is way too long to copy/paste. This is a trimmed example of what is going on – user973479 Apr 26 '12 at 16:01
  • 1
    Is this about a particular IDE? Because from the compiler standpoint I do not see anything wrong with this. – Edwin Dalorzo Apr 26 '12 at 16:04
  • 1
    @user973479 afaik this warning should only appear if doFoo() is a method of Bar itself. So either your example or your IDE is wrong. – josefx Apr 26 '12 at 16:05
  • 1
    @user973479 What IDE are you using? Neither IntelliJ nor javac nor eclipse does give me a warning there and quite clearly there's no reason for `doFoo` to be final - it only makes sense if we're calling an non-final instance method of the `constructed` instance. – Voo Apr 26 '12 at 16:10
  • 1
    I was using SONAR to do code coverage and got that message for this code. I guess perhaps there's a bug in SONAR? – user973479 Apr 26 '12 at 16:14
  • If the above code gives a warning in Sonar, yes that's most certainly a bug. If updating to the newest version doesn't fix it, you should file a bug report with the devs. – Voo Apr 26 '12 at 18:14

4 Answers4

20

As @Voo says,

your question is about calling a virtual method on an already completely constructed object. The well known downfalls of calling virtual methods on the to constructed object are well known, but don't apply here

From Effective Java 2nd Edition, Item 17: Design and document for inheritance, or else prohibit it:

There are a few more restrictions that a class must obey to allow inheritance. Constructors must not invoke overridable methods, directly or indirectly. If you violate this rule, program failure will result. The superclass constructor runs before the subclass constructor, so the overriding method in the subclass will be invoked before the subclass constructor has run. If the overriding method depends on any initialization performed by the subclass constructor, the method will not behave as expected.

Invocation of an overridable method during object construction may result in the use of uninitialized data, leading to runtime exceptions or to unanticipated outcomes.

Constructors must invoke only methods that are final or private

You could use static factory methods to fix the problem that you have to create your objects from the Bar class.

Effective Java, Item 1: Consider static factory methods instead of constructors

The normal way for a class to allow a client to obtain an instance of itself is to provide a public constructor. There is another technique that should be a part of every programmer’s toolkit. A class can provide a public static factory method, which is simply a static method that returns an instance of the class.

So, you go to have the interface :

public interface Foo {
     void doFoo();
}

and the implementation:

public class FooImpl implements Foo {
   @Override
   public void doFoo() {
   //.. Do important code
   }
}

To create your class with your factory method you could work by this way:

  • Use the interface to define the variable of your class private Foo fi instead of private FooImpl fi, using interfaces over concrete types is the key for good encapsulation and for loose coupling your code.

  • Make your default constructor private to prevents instantiation of your class outside.

    private Bar() { // Prevents instantiation }

  • Remove all calls to override methods that are present in your constructor.

  • Create your static factory method

Finally you get a class Bar with a factory method like :

public class Bar {
    private Foo fi;

    private Bar() {// Prevents instantiation
        fi = new FooImpl();
    }

    public static Bar createBar() {
        Bar newBar = new Bar();
        newBar.fi.doFoo(); 

        return newBar;
    }
}

My Boss says: “the Sonar warnings are about symptoms, not about the disease. It’s best when you can treat the disease.” !

Jesus Zavarce
  • 1,729
  • 1
  • 17
  • 27
6

You could declare doFoo as final if you don't need to override that method later:

public final void doFoo() { }

Pablo
  • 3,655
  • 2
  • 30
  • 44
0

Your IDE is telling you that, because it is potentially unsafe. You can provide any implimentation or doFoo and make all Bar objects to different stuff on startup. This seems like a bad choice of design in most cases.

It seems like you are using a strategy pattern, in a constructor. It's not wise to use a strategy or any other overidable behaviour in the constructor. Use it some place else.

Maarten Blokker
  • 604
  • 1
  • 5
  • 23
  • How is it potentially unsafe to call a non final method of a completely different class in the constructor? – Voo Apr 26 '12 at 16:10
  • Your downvoting my answer because you do not understand the implications followed by overridable behaviour in a constructor? This article explains the potential danger of using a calling an overridale method in a constructor: http://www.javapractices.com/topic/TopicAction.do?Id=215 – Maarten Blokker Apr 26 '12 at 17:04
  • 1
    If you had actually read my post you would've noticed that the question is about calling a virtual method on an already completely constructed object. The well known downfalls of calling virtual methods on the to constructed object are well known, but don't apply here. – Voo Apr 26 '12 at 18:12
  • i can see that you are using a concrete implementation, but your ide can not ;) THIS is why you see the message – Maarten Blokker Apr 26 '12 at 18:47
  • If your IDE can't differentiate a call on a local method (which can only be one of the following: `this.foo()`, `super.foo()` or `foo()`) from calling a method on an instance, you've got a real problem. Also no, javac, intellij and eclipse all get it right, but the OP already specified that Sonar is giving the warning - clearly a bug in Sonar. – Voo Apr 26 '12 at 18:52
0

The source of the error you see is PMD (search there for "overr"), and when constructing your example again, this warning is not triggered by this version of PMD (4.2.6). Sonar just integrates PMD, Checkstyle and other tools, and provides an overview. So check which version of Sonar (and PMD) you are using.

You can look at that in Sonar: Sonar > Quality Profiles > Search for "overr" should highlight the rule you are using.

In Sonar, you can check which version of PMD you are using. Go to Sonar > Configuration > Update Center, and look there at the version of PMD you are using.

mliebelt
  • 15,345
  • 7
  • 55
  • 92