Sonar "useless assignment to local variable" workaround?

I am working towards improving my code, and I came across this issue from Sonar:

Remove this useless assignment to local variable "uiRequest" 

Fact is, it is not useless, as I am using it just after in the code:

 // I am supposed to remove this UiRequest uiRequest = null; if("Party".equals(vauban.getName())) { uiRequest = contextBuilder.buildContext(vauban); } else { // Maybe I could work my way around here ? throw new NamingException( String.format( "Hey %s, change your name to %s, thanks", vauban.getName(), "Vauban")); } // Set the generated Id in the result of context builder MyOwnService response = callService(uiRequest, vauban); return response; 

Sonar still tells me that "uiRequest" is useless, why ? It is not, as I don't want it to reach the code if it is null. I tried initializing it (uiRequest = new UiRequest()) but it keeps telling me that it is useless.

Anyone got an idea about why Sonar behaves like this / how to correct this ?

4 Answers

Your issue simplifies to:

Foo x = null; if(a()) { x = b(); } else { throw new Exception(); } c(x); 

There are two potential paths through this code:

1. a() returns true. x is assigned b() then c(x) is called.
2. a() returns false. An exception is thrown and c(x) is not called.

Neither of these paths calls c(x) using the initial assignment of null. So whatever you assigned at first, is redundant.

Note that this would also be an issue if the initial assignment was something other than null. Unless the right-hand-side of the assignment has a side effect, any assignment is wasted. (Sonar analyses for side-effects)

This is suspicious to Sonar:

• Maybe the programmer expected the first assignment to have an effect -- it doesn't, so perhaps it's a bug.
• It's also about code clarity -- a future human reader of the code might waste time wondering what the initial value is for.
• If the right-hand-side had involved computation, but had no side effect, it would be wasted computation.

You can fix this in two ways:

Firstly just removing the = null, leaving Foo x; - Java is clever enough to realise that all routes to c(x) involve an assignment, so this will still compile.

Better yet, move c(x) into the block:

if(a()) { Foo x = b(); c(x); } else { throw new Exception(); } 

This is logically equivalent, neater, and reduces the scope of x. Reducing scope is a good thing. Of course, if you need x in the wider scope, you can't do this.

One more variation, also logically equivalent:

if(! a()) { throw new Exception(); } Foo x = b(); c(x); 

... which responds well to "extract-method" and "inline" refactorings:

throwForInvalidA(...); c(b()); 

Use whichever communicates your intent best.

You can avoid the warning and perhaps have slightly more readable code by:

// I am supposed to remove this // UiRequest uiRequest = null; <-- remove // invert the test here if(! "Party".equals(vauban.getName())) { // Maybe I could work my way around here ? throw new NamingException( String.format( "Hey %s, change your name to %s, thanks", vauban.getName(), "Vauban")); } // you are only using the variable in the call service; make // final since reference should not change after assignment final UiRequest uiRequest = contextBuilder.buildContext(vauban); // Set the generated Id in the result of context builder MyOwnService response = callService(uiRequest, vauban); return response; 

What about moving it into the if statement?

 if("Party".equals(vauban.getName())) { UiRequest uiRequest = contextBuilder.buildContext(vauban); // Set the generated Id in the result of context builder MyOwnService response = callService(uiRequest, vauban); return response; } else { throw new NamingException( String.format( "Hey %s, change your name to %s, thanks", vauban.getName(), "Vauban")); } 

Or throw exception in the if statement and just have no else clause. i.e let the normal case be the "happy path".

The assignment is useless because no code can possibly see that assigned value.