squid:S3655: Optional in as member of a class

Clean Code Report False-positive / False-negative...
1

Consider this code:

import java.util.Optional;

public class SampleClass {

    public Optional<String> opt;

    public void setOpt(Optional<String> opt) {
        this.opt = opt;
    }

    public Optional<String> getOpt() {
        return opt;
    }
}

Then in an other class:

public static void main(String[] args) {
    SampleClass s = new SampleClass();
    String value;
    
    if(s.opt.isPresent()) {
        value = s.opt.get(); //Error reported by squid:S3655, Optional value should only be accessed after calling isPresent()
    }
}

The same if you write it like this:

public static void main(String[] args) {
    SampleClass s = new SampleClass();
    String value;
    
    if(s.getOpt().isPresent()) {
        value = s.getOpt().get(); //Error reported by squid:S3655, Optional value should only be accessed after calling isPresent()
    }
}

In my opinion there should be no error for those cases.

2

Hi,
I don’t think it is a bug. Now anybody is able to do this:

SampleClass obj = new SampleClass();
obj.setOpt(null);

if (obj.getOpt().isPresent()) {
    // NPE has been thrown in the above line
}

This construction is evil :wink:

You should do something like this:

import java.util.Optional;

public class SampleClass {

    private Optional<String> opt = Optional.empty();

    public void setValue(final String value) {
        opt = Optional.ofNullable(value);
    }

    public Optional<String> getOpt() {
        return opt;
    }
}

or

import java.util.Optional;

public class SampleClass {

    private String value;

    public void setValue(final String value) {
        this.value= value;
    }

    public Optional<String> getOpt() {
        return Optional.ofNullable(value);
    }
}

and then

SampleClass obj = new SampleClass();
obj.setValue(null);

if (obj.getOpt().isPresent()) {
    // no NPE
}

Cheers

3

Thank you for your reply,

I think you are right about my example not being perfect.
Forget about my setOpt(…) method. This is bad practice, I agree with you.

Lets continue the discussion with the version you have proposed:

import java.util.Optional;
public class SampleClass {

    private String value;

    public void setValue(final String value) {
        this.value= value;
    }

    public Optional<String> getOpt() {
        return Optional.ofNullable(value);
    }
}

I have the feeling you miss the point I wanted to discuss here.
It is about the squid:S3655 rule.

This reports an error:

public static void check(Optional<String> opt) {
    String v = opt.get(); //Error reported by squid:S3655, Optional value should only be accessed after calling isPresent()
}

This does not report any error:

public static void check(Optional<String> opt) {
    if(opt.isPresent()) {
        String v = opt.get();
    }
}

But now, if the Optional can be accessed with a getter of the class class (take your corrected SampleClass example), the check of rule squid:S3655 reports a false positive:

public static void check(SampleClass s) {
    if(s.getOpt().isPresent()) {
        String value = s.getOpt().get(); //Error reported by squid:S3655, Optional value should only be accessed after calling isPresent()
    }
}
4 
public static void check(SampleClass s) {
    if(s.getOpt().isPresent()) {
        String value = s.getOpt().get(); //Error reported by squid:S3655, Optional value should only be accessed after calling isPresent()
    }
}

I don’t think this is a false positive. The getOpt may return different object every time. You should cache the value:

public static void check(final SampleClass s) {
    final Optional<String> opt = s.getOpt();
    if (opt.isPresent()) {
        String value = opt.get();
    }
}