[java:s1117] FP when just fetching value t oa local variable

SonarQube Server / Community Build
, ,
1

Qube: Community 9.9

There is a pretty common FP for the Java rule s1117.

private String foo = "bar";

public String getFoo()
{
    return foo;
}

private void test()
{
    final String foo = getFoo();
    
    doSomething( foo );
}

Very often I only want to fetch the result of a plain getter into a local variable or constant. If the variable is defakto constant or at least if it is actually marked final, the rule should not hit, since the variable name might hide the local member, but it is actually the same (immutble) value.

It doesn’t make much sense to use another name for the local variable. And I think, this is a pretty common use case.

4

Hi Marvin, thanks for the report.

I feel like considering this an FP is a little bit of a stretch: the analyzer cannot know statically what the method getFoo() will return, and saying that is a “plain getter” is a convention that is not compulsory in Java ( you can have a class with a field private String x; and a method getX() that does not return that string).

Consider also the case where you retrieved the value with getFoo(), and you have shadowed the class field with the new final String foo, if you want to modify the class field now you will have to do something like this.foo = "another string", which will deteriorate the code readability.

Personally, I would suggest giving another name to the local variable inside the method test(), something like final String localFoo.

6

I think this would make me disable this rule. What a pity. Just too many FPs.

I really think, Sonar can very well know statically that this method returns the object member foo. It does for other rules as well, where it complains about the method not returning the expected value considering the name. And here I am not talking about the getter getFoo() being expected to return foo. But Sonar can know statically, that the lokal method is a getter to foo. And then this exception is not only very well possible, but also correct.

If you fetch foo into a local variable, there is little to no need to access this.foo btw.

7

I thought a little bit more about this rule, because it really produces a lot of FPs here. Though it is still very useful for some cases and especially other people here.

My suggestion now is as follows.

The rule should not hit, if all these conditions apply.

  • The local variable is fetched through a plain getter, that does not do more than returning the member. The getter’s name does not matter here. But it returns the member unmodified and only this.
  • The local variable is declared final. Or, if that is feasible for the rule to detect, it is defacto final as no modifications are applied to the variables value (the pointer for objects or value for primitives).

Maybe this would be good for a rule option. But I think, if these minimalistic conditions are met, we can eliminate the most of the annoing FPs. And it can be activated unconditionally, because it does not make anything worse or weaker, but just improves the results a bit. All other cases can be very well handled by using another name for the local variable.

What do you think?