New Java Check : Call to Enum.ordinal()

[Michel Pawlak]

Hi again,

Here follows another new Java check proposition. Again, please tell me if you would be interested by it.

Michel

Definition :

@Rule(

    key = "",

    name = "Call to Enum.ordinal()",

    description = "Rule that checks that no calls to Enums ordinal() method are done as it is highly error prone and may result in unexpect runtime behavior in case of Enum refactoring.",

    priority = Priority.BLOCKER, // in our context it has to be considered as BLOCKER, but CRITICAL may be high enough

    tags = {

        "reliability"

    })

@SqaleMetadata(

    characteristic = RulesDefinition.SubCharacteristics.RELIABILITY_COMPLIANCE,

    remediationCostOffset = "15min",

    remediationFunctionType = DefaultDebtRemediationFunction.Type.CONSTANT_ISSUE)

Test oracle file :

public class AClass {

  public class AnotherClass {

    public int ordinal() {

      return 1;

    }

  }

  

  public enum AnEnum {

    A, B, C;

  }

  

  public int legitCall() {

    return new AnotherClass().ordinal();

  }

  public int forbiddenCall() {

    return AnEnum.A.ordinal(); // Noncompliant {{Do not call java.lang.Enum.ordinal() method as it is subject to bugs in case of enum refactoring and values reordering.}}

  }

}

[G. Ann Campbell]

Hi Michel,

Here's the first draft of the rule specification: https://jira.sonarsource.com/browse/RSPEC-3351

Note that I've set the severity to Major.

The first step in getting a pull request accepted for this rule, is aligning your implementation with the RSpec (once we agree on it, that is :-))


Ann

[Michał Kordas]

Hi Ann,

Just a bit of feedback to RSPEC example: enums are usually compared using == instead of equals. Two enum constants are equal only when they have the same identity (http://stackoverflow.com/questions/1750435/comparing-java-enum-members-or-equals)

Thanks,

Michal

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/877e3d0d-8da6-4d59-bd48-b702ce5d3e84%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Michel Pawlak]

Hi Michal,

Isn't a developer's first mission to write working, readable, tested and maintainable code ? Optimizing code come later.

Let me quote some some text from the post you mention about "==" usage:

enums are Objects. For all other Object types the standard comparison is .equals(), not ==

This one is really important and is the core reason why equals should be used.

It works: Until you need to replace an enum with a class as part of a refactoring. Good luck finding all the code using == that breaks.

Indeed it works, Enums equality can be verified using ==, by the way that's what is done under the hood by all (most?) VMs, but it is done for optimization reasons not for readability / maintainability reasons, and it is hidden to the enduser.

 

It's faster: Even if true (questionable), there's that Knuth quote about premature optimization.

 

Same as previous comment. Such low level optimizations should be left to the compiler + JIT compiler. And seriously "how much" faster ? with a -client or -server ? for how many hits ? And I would like to ask people doing such low level optimizations if they are sure that all their algorithms are so perfect they need to do such kind of optimization (I'm pretty sure JIT does them as well anyway)

Safer at run-time: "Safe" is not the first word that springs to mind if the use of == is the only thing separating you from a NullPointerException.

Agree, and SQ can help you write correctly the equals statement to make sure that NPE will never be thrown. You can also use @NonNull which makes the code more readable and understandable (and thus maintainable, ... and more secure in the end)

Safer at compile time: Not really. It can guard you from the rather elementary mistake of comparing the wrong types, but again, if that's how dumb your programmers are, "safe" is the one thing you're not.

I Agree (at least partially... it's not about being dumb but about making stupid mistakes anyway, but well... sleepy people can also use = instead of == in other situations)

All in all, yes, you can check enums' equality with == but it is not worth the impact on readability and maintainability.

In other words I would not change your example @Ann.

Michel