PMD : Avoid using Literals in Conditional Statements

Question

I have following code. I am getting "Avoid using Literals in Conditional Statements." warning in PMD on line number 5.

List<Object> listObj = getData();
 if (listObj.isEmpty()) {
      throw new NoEntity("No entity found for given Device.");
 }
 if (listObj.size() > 1) {
      throw new MultiEntity(
          "Multiple entity record found for given Device.");
 }

I prefer not to put global static final int variable having value as 1 and use it in if condition. Any other solution for this ?

Seldun

As with all hammers, PMD is a rather large hammer, you need to make sure you are hitting nails. In this case, I think the code you have is perfectly fine and you should suppress the warning. – Pirate

You can suppress the worning but it will be easy to manage code in future if you will create static final constant for "1" . Its a standered practice. – Disarticulate

well if you want to be sneaky you could remove the 1st element from the list and then check again with isEmpty()! However that would be an obtuse thing to do. If you don't want to declare a static variable then you must suppress the warning. This check is one of the controversial ones so don't feel bad about suppressing it. – Lavettelavigne

@NiravChhatrola Are you suggesting that I should create static final constant for "1" in this class ? If so, what should I do if get same warning in other class? – Seldun

If your requeirement is like that you have to use that constant multiple times in different files ... than its better practice to create one interface for constanct and decleare all constants in it. interface has defacult public static final modifire for veriables. – Disarticulate

if (listObj.iterator().next().hasNext()) but that would get -99 when posted as answer. Philosophical: if (listObj.size() != Byte.BYTES) { throw new ... + listObj.size() – Stolon

Answer 1

If you are using Apache Commons Lang, it's available in NumberUtils https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/math/NumberUtils.html#INTEGER_ONE

Allowed

Yes, this would make the warning disappear, however the entire point of warning is to make conditionals easier to understand. If you change 1 with NumberUtils.INTEGER_ONE still won't make the code any better. If you create a constant named i.e. private static final int MAXIMUM_NUMBER_OF_LIST_ELEMENTS = 1; will make it more understandable for future developers why you used the number 1. – Astonied

I disagree, the check itself makes it clear that the a list with more than 1 elements is not acceptable, in fact, INTEGER_ONE is more precise here. Moreover, the exception thrown immediately after the check clearly specifies the violation "Multiple entity record found for given Device." You might define a new constant if the semantics of the conditional changes – Allowed

Coincidentally we are using Apache Commans Lang for other purpose, so we are going to use it for this case too. Thanks. – Seldun

It is very common to handle zero, one and n as separate cases and entirely reasonable to use literals for 0 and 1 which are shall we say widely understood values. Sometimes the PMD warnings are pedantic and useless. – Porcine

Answer 2

You can insert an assignment before the conditional, with a nice variable name that will serve as documentation. You'll improve the readability and make the warning go away.

boolean multiEntityDevice = listObj.size() > 1;
if (multiEntityDevice) {
  throw new MultiEntity(
      "Multiple entity record found for given Device.");
}

Forego

Answer 3

The best solution is to suppress the warning is such simple cases with @SuppressWarnings("PMD.AvoidLiteralsInIfCondition").

However, for your example I have a solution.

Use Guava's Iterables:

List<Object> listObj = getData();
try {
    Object myObj = Iterables.getOnlyElement(listObj);
} catch (NoSuchElementException e) {
    // "No entity found for given Device."
} catch (IllegalArgumentException e) {
    // "Multiple entity record found for given Device."
}

Astonied

+1 for suppressing the warning. -1 for the Guava "solution": this is just another workaround that prevents the warning but makes the code more difficult to understand (still better than NumberUtils.INTEGER_ONE though). – Weatherspoon

Answer 4

You can modify the rule definition in your PMD configuration to include 1 as a MagicNumber. I don't see any reason why 1 should not be one of the default values in the rule itself.

<rule ref="category/java/errorprone.xml/AvoidLiteralsInIfCondition">
    <properties>
        <property name="ignoreMagicNumbers" value="-1,0,1" />
        <property name="ignoreExpressions" value="true" />
    </properties>
</rule>

If you don't like having to modify the rule configuration yourself, you can raise an issue with PMD on Github.

Koss