26

I have the following class Properties:

class Properties {
    private Boolean enabled;

    public Boolean getEnabled() {
        return enabled;
    }
}

If I write the following code, SonarLint gives me a warning on the if condition saying "Use the primitive boolean expression here.".

if (!properties.getEnabled()) {
    return true;
}
// more code

Changing the if condition to the following shuts up the warning. But that less readable, that can't be what SonarLint wants or?

if (properties.getEnabled().equals(Boolean.FALSE)) {
    return true;
}
// more code

What exactly does SonarLint want me to do here? What is the problem?

findusl
  • 2,454
  • 8
  • 32
  • 51
  • 4
    What does it mean for `enabled` to be `null`? https://imgur.com/gallery/80Indtp – Andy Turner Nov 25 '19 at 15:36
  • 2
    Maybe declare your *enabled* attribute as a primitive boolean. I think Sonarlint is trying to prevent a Null Pointer Exception – D. Lawrence Nov 25 '19 at 15:36
  • 2
    @D.Lawrence Indeed, putting a null check before it does silence the warning. A useful warning, but a very confusing message. Thank you – findusl Nov 25 '19 at 15:40
  • 1
    @AndyTurner nice image. It should not be null, it is annotated with NotNull as well. However because of the framework that sets the value it is a non primitive. Yeah not good framework one may argue and one may be right. But not really my choice. With Lawrences tip SonarLint also accepts it. Thanks you for trying and responding so quick – findusl Nov 25 '19 at 15:42

5 Answers5

37

As other already mentioned, Sonar wants you to make sure that you don't have any null pointer exception, or at least that's what see when I do a null check before trying to validate against the variable:

if I have the next code, Sonar raises an alert that this code can throw a null pointer exception.

if (properties.getEnabled()) {
       // Your code
}

But if I add a quick validation against nulls, Sonar stops complaining about it:

if (properties.getEnabled() != null && properties.getEnabled()) {
       // Your code
}

Now, as you mentioned you can use the Boolean class to compare Booleans like the next code:

Boolean.TRUE.equals(properties.getEnabled());

And you can place it in an if statement like the next code:

if (Boolean.TRUE.equals(properties.getEnabled())){
       // Your code
}

This might look like it's too verbose, but the implementation of Boolean.TRUE.equals() checks if the object is an instance of the Boolean class, and null cannot be an instance of any class, as null is not an instance. You can find it better explained here: Is null check needed before calling instanceof?

They used to have a very nice and easy set of test to understand what was accepted and what's not that I published in this answer when I first answered it but it looks like it does no longer exists in master (I still add it for an easier understanding but this is an old version of sonar so it should be used only as reference): https://github.com/SonarSource/sonar-java/blob/5.14.0.18788/java-checks/src/test/files/checks/BoxedBooleanExpressionsCheck.java

You can reference the Implementation code as well for this rule here: https://github.com/SonarSource/sonar-java/blob/master/java-checks/src/main/java/org/sonar/java/checks/BoxedBooleanExpressionsCheck.java#L131

Damonio
  • 512
  • 5
  • 8
  • Interesting that sonar would accept the null check as you declared it, because a getter is just a function that can return a different value on every call. – findusl Jan 29 '20 at 08:25
  • I don't see why sonar won't allow me to, that's the reason that you want to verify that the getter doesn't return a null, since in every case there might be a possibility that your object contains true/false/null, only in runtime you will know the value – Damonio Jan 29 '20 at 15:27
  • but you do not clarify it. The first call to getEnabled may return something else than the second call. in order to be save here you'd have to save the result to a local variable and then work with that one. – findusl Jan 29 '20 at 16:13
  • I get the point that it might be different but even if you save the result to a local variable, you are only going to store the reference to the Boolean, not the actual value, in which that can change if another thread uses the same object. It might be that the reason is that the object was within the scope of a function and not a member variable, which for sonar, it was guaranteed that no other thread will modify that object. But if there might be a thread insecurity then, as you mentioned storing the actual value in a local variable would be the best. – Damonio Jan 29 '20 at 19:54
1

Use org.apache.commons.lang3.BooleanUtils, it's a null safe way:

if (BooleanUtils.isNotTrue(properties.getEnabled())) {
    return true;
}
saffer
  • 111
  • 4
0

This seems to have a simple answer. If the property is nullable, then the sanity at getter should help.

class Properties {
    private Boolean enabled;

    public Boolean isEnabled() {
        return enabled == true;
    }
}
Mukund Desai
  • 100
  • 2
-1

Try .booleanValue() Like this if(properties.getEnabled().booleanValue()) { } Hope so it will help u.

Rohit Chaurasiya
  • 446
  • 4
  • 8
  • It will throw null pointer if `properties.getEnabled()` returns a null – Snigdhajyoti Sep 14 '21 at 11:31
  • Indeed there is a possibility of exception, and also, for me, I think IntelliJ complains about `Unnecessary unboxing 'properties.getEnabled().booleanValue()'` then I still have a visible issue in editor. – gluttony Oct 28 '22 at 14:42
-3

try this :

public boolean x (){ 
    boolean prop=properties.getEnabled() 
    if (prop) { 
        return true; 
    }else return false; 
}
Suraj Rao
  • 29,388
  • 11
  • 94
  • 103
fady manaa
  • 7
  • 1