CODEWITHSUNDEEP
javasonarqube
Davi
Davi

Reputation: 9

SonarQube warning "Methods returns should not be invariant"

SonarQube says doing following is wrong:

@Override
  public boolean android.os.Handler.Callback.handleMessage(Message msg)
  {
    super.handleMessage(msg);
    Object obj = getXY();
    if (obj == null) { return true; }
    ...
    ...
    ...
    ...
    ...
    ...
    return true;
}

When a method is designed to return an invariant value, it may be poor design, but it shouldn't adversely affect the outcome of your program. However, when it happens on all paths through the logic, it is surely a bug.

This rule raises an issue when a method contains several return statements that all return the same value.

I don't think this warning is true in this case, what do you think? Would you have a different approach?

Upvotes: 0

Views: 3111

Answers (3)

Ralf Kleberhoff
Ralf Kleberhoff

Reputation: 7290

TL;DR You are the developer in charge, and SonarCube is your assistant. You make the decisions, but it's a good idea to at least listen to your assistant.

Whenever someone (SonarCube) tries to formalize typical programming mistakes into detection rules, there will be false positives, i.e. situations where the rule matches, but the code is okay.

In your case, the "typical programming mistake" is e.g. to write a boolean-returning method where the outcome should depend on some parameters, but you mistakenly always return the same value. Such mistakes happen.

A warning means that SonarCube found suspicious code, and you should look at the source code fragment, and decide whether there is a mistake or not. SonarCube gives valuable hints. You should take the hints seriously, try to understand what issues they address. In many cases, the hints really indicate a mistake, a design flaw, or bad style. But if not (and it's you who decides), leave the code unchanged.

Coming back to your case: If always returning true and having some early-return statements is intended here, and adheres to your style guide, you should tell SonarCube so (but others have to answer how this can be done, maybe using a @SuppressWarnings annotation).

Others suggested to change the code, replacing the early-return with nested conditionals (assuming that then the method will pass SonarCube), but I recommend against that:

  • It deviates from the programming style that you seem to be used to.
  • Such a refactoring implies the risk to introduce new mistakes. Human beings aren't very good in understanding complex conditionals, so it's easy to mess things up in the process.

Upvotes: 1

MC Emperor
MC Emperor

Reputation: 22967

Well,

This rule raises an issue when a method contains several return statements that all return the same value.

Sonar reports that it complains about all return statements having the same return value. Your code indeed contains (at least) two such statements:

if (obj == null) { return true }
...
return true;

I think that you missed a semicolon there, but that's another story.

You could at least rewrite this to:

if (obj != null) {
   ...
}
return true;

Now see if the complaint disappears. If it does not disappear, there's not much you can do. As Federico already pointed out in a comment, Sonar is a very nice tool, but it is not the holy grail. There are cases where you have to tell Sonar that this is the only way.

Regarding some comments saying that you should change the return type to void — I would advise you the same, but you cannot do that, or course, if you are overriding from a supertype. (I'm assuming here that you are overriding android.os.Handler.Callback.handleMessage(Message)).

Upvotes: 2

Sandeep Nair
Sandeep Nair

Reputation: 456

The flag here does not represent anything as it will always be true. When will it be false. If you want to execute some logic based on whether object is null or not then its better to have the method return void and write some logic like this. Is the interface or abstract class your own and you only have such cases where the flag is just a formality then I would suggest to change the method in interface or abstract class to have the method return void and do in the implementation something like following

@Override
  public void handleMessage(Message msg)
  {
    super.handleMessage(msg);
    Object obj = getXY();
    if (obj != null) {
    ...
    ...
    ...
    ...
    ...
    ...
    }
}

Upvotes: 0

Related Questions