10

I have this code:

public static class MyWebDriver extends RemoteWebDriver {
    @NotNull
    private final String nodeId;

    public MyRemoteWebDriver(@NotNull String nodeId) {
        super();
        this.nodeId = nodeId;
    }

    @Override
    public void quit() {
        System.out.println("deleting node: " + nodeId);
    }
}

and it's guaranteed that nodeId that is passed into constructor is not null. And because nodeId field is final I expect it to be initialised in my quit() method.

But, in super() constructor there is a try-catch block which in case of exception calls quit() method and throws an exception. And in this case nodeId that I get in my quit() method is not initialised (has null value).

Are there any ways of avoiding it except

@Override
public void quit() {
    if (nodeId != null) {
        System.out.println("deleting node: " + nodeId);
    }
}

this one? Which looks pretty stupid, because nodeId is marked as @NotNull.

esin88
  • 3,091
  • 30
  • 35
  • I just put `System.out.println()` instead of my actual code, that relies on `nodeId` value. That's why I want to avoid it. – esin88 Mar 10 '17 at 13:23
  • And just to clarify: `RemoteWebDriver` class is not mine, it's from `org.openqa.selenium.remote` package. – esin88 Mar 10 '17 at 13:28
  • 1
    @esin88 so `openqa` project (whatever that is) calls a `non-final` method from within the constructor? seems like this should be a bug declaration, which obviously at the moment does not make your life easier – Eugene Mar 10 '17 at 13:46
  • @esin88 you will not get a better **and simpler** answer then what you have already done (with the if check)... I would stick to that, but opening a bug and adding a code comment – Eugene Mar 10 '17 at 13:48

4 Answers4

4

But, in super() constructor there is a try-catch block which in case of exception calls quit()

This are two problems in one:

  1. constructors should not do any work except storing the given parameters in (final) member variables.

    [edit]

    Does that mean classes should not validate their inputs? I know many people who would disagree with having all of their objects possibly invalid instead of never existing at all. – chris

    By "should not do any work" I mean constructor should not calculate any property value from the parameters or call a dependency or a non public method to do the check.

  2. constructors should never call any other than private and/or final methods (within the class), either directly or indirectly (i.e. you mustn't call a final method which in turn invokes a non-final method).

The reason why you run into this problem is the violation of the single responsibility Pattern and Separation of Concerns.

What ever you do in the super classes constructor should most likely be done in a separate class and only the results of that process should be passed into your class (and its super class).

This obviously means that the quit() method also belongs to a different class.

Andy Turner
  • 137,514
  • 11
  • 162
  • 243
Timothy Truckle
  • 15,071
  • 2
  • 27
  • 51
  • Unfortunately, I can't alter `super()` constructor behavior, because it's from third-party library. – esin88 Mar 10 '17 at 13:31
  • 1
    *constructors should not do any work except storing the given parameters in (final) member variables.* - Does that mean classes should not validate their inputs? I know many people who would disagree with having all of their objects possibly invalid instead of never existing at all. – chris Mar 10 '17 at 13:34
  • @esin88: If you can't avoid to override the `quit()` method you are left with the null check... – Timothy Truckle Mar 10 '17 at 13:37
  • @chris *"that mean classes should not validate their inputs?"* This depends on what you validate. In your constructor you should only do validations you can do without accessing any dependency. Any other validation should be done before passing the inputs to a constructor. – Timothy Truckle Mar 10 '17 at 13:41
  • 4
    "constructors should never call non private methods" constructors should never call non-*final* methods, directly or indirectly. The non-private-ness isn't the issue, it's the potential for the method to be overridden. – Andy Turner Mar 10 '17 at 13:45
  • @AndyTurner *"The non-private-ness isn't the issue, it's the potential for the method to be overridden."* thanks for pointing at this, I'm changing my answer. – Timothy Truckle Mar 10 '17 at 13:48
  • @TimothyTruckle, Right, good to know what you're arguing in more detail. – chris Mar 10 '17 at 14:22
  • 1
    @AndyTurner Thanks for improving my answer. – Timothy Truckle Mar 12 '17 at 10:29
2

This is of course easily explained by the fact how Java works. An object of type MyWebDriver is not initialized, including its fields, i.e. including in your case nodeId until its fields inherited from the super class are initialized, i.e., in your case, until super() returns.

So if an exception in super is thrown, clearly nodeId will be null.

I don't think there's any solution (except some similar to what you're suggesting) unless the framework you're using (which you didn't specify) provides some workaround.

nbro
  • 15,395
  • 32
  • 113
  • 196
1

You can move initialization logic from super constructor to protected init() method and invoke it after nodeId initialization.

Maybe clearer design can be used here but it's hard to suggest anything without full example.

Andrey Koshelev
  • 211
  • 1
  • 4
  • Unfortunately, I can't alter `super()` constructor behavior, because it's from third-party library. – esin88 Mar 10 '17 at 13:31
  • 2
    Then it may be best to use Adapter pattern and move 3d party class inside your class implementation instead of extending from it. – Andrey Koshelev Mar 10 '17 at 13:38
0

You should change bean behind RemoteWebDriver, adding to quit this boolean comparison. Or even better, you shouldn't call any methods on your constructor, only construct the instance itself setting values. After that, evaluates the value of a new instance variable boolean isItFullyConstructed and if false, call the original try/catch logic that was causing trouble. Is this doable?

rado
  • 5,720
  • 5
  • 29
  • 51