Sonar "useless assignment to local variable" workaround?

Question

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 ?

Answer 1

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.