[java] Improve ConstantsInInterface message to mention alternatives

[krichter722]

Affects PMD Version: 6.5.0-SNAPSHOT

Rule: ConstantsInInterface

Description:

Avoid constants in interfaces. Interfaces should define types, constants are implementation details better placed in classes or enums. See Effective Java, item 19.

suggests to place constants in enums. I assume that doesn't imply to remove the static modifier and was wondering how that's possible since placing anything before the enum listing causes an identifier expected compilation error and placing them after the listing causes an illegal forward reference (compilation) error. Placing them in a private interface as suggested (as only solution) in https://stackoverflow.com/questions/23608885/how-to-define-static-constants-in-a-java-enum causes the ConstantsInInterface to fail again. It'd be nice to see the solution from Effective Java in the docs (it shouldn't be an issue to rephrase it and add a citation to the original).

Code Sample demonstrating the issue:

I can provide one if it's really necessary.

Running PMD through: Maven

[jsotuyod]

Effective Java's item 19 (22 in the 3rd edition, using the name of item that remains unchanged may be better than using the number) states:

If the constants are best viewed as members of an enumerated type, you should export them with an enum type (see item Use enums instead of int constants)

If they are not, a constant utility class (private constructor) is preferred.

It's not that both are equivalent, but rather that you should use enums if constants describe enumerations, or a constant utility class otherwise.

[krichter722]

I'm sorry, I don't get it. My point is, assuming we have

public interface BonusConstants {
    int REWARD = 5;
}

public enum Permission {
    SMALL_JACKPOT(10*BonusConstants.REWARD),
    BIG_JACKPOT(50*BonusConstants.REWARD);

    private final int minimum;
    [constructor and getter]
}

public class BonusCalculator {
    public int calcRewardDiff(int points) {
        for(Permission permission : Permission.values()) {
            int difference = permission.getMinimum() - points;
            if(difference > 0) {
                return difference; //you need that many points for the next jackpot
            }
        }
        return -1; //you've had access to all jackpots
    }
    //The implementation should do more in order to be correct, but illustrates my point
}

where do we put REWARD? In BonusConstants it causes ConstantsInInterface failure (depending on the configuration). We can't put it in Permission because it causes the compilation errors mentioned in the question.

Apparently, the point of the quoted book passage is to put REWARD into a utility class rather than an interface. It should be explained why this is better coding (I don't see it in the idea and the failure message). The reference to enums seems obscure because of the impossibility in certain cases to place the constant into the enum (see above). It'd be great if the new message or details would explain both (in the sense of "Don't make me think").

[jsotuyod]

In that particular case, a utility class is your best option. Moreover, if only the enum is using it, I'd use a inner static utility class to avoid exporting it altogether:

public enum Permission {
    SMALL_JACKPOT(10*BonusConstants.REWARD),
    BIG_JACKPOT(50*BonusConstants.REWARD);

    private final int minimum;
    [constructor and getter]

    private static class BonusConstants {
        public static final int REWARD = 5;
        private BonusConstants() { }
    }
}

As for the rule in general, consider the scenario:

public interface WeatherConstants {
  int SUNNY = 0:
  int CLOUDY = 1;
  int RAINY = 2;
  int THUNDERSTORM = 3;
}

In this case, the constant values (0, 1, 2, 3) have no real meaning. This is a poor-man's enumeration, hence, as the book states:

If the constants are best viewed as members of an enumerated type, you should export them with an enum type

Meaning, we should favor writing:

public enum WeatherTypes {
  SUNNY,
  CLOUDY,
  RAINY,
  THUNDERSTORM;
}

For other scenarios (as your own), the utility class is the best alternative. The book provides a number of reasons for this. The first several deal specially with the scenario where such constants are referenced "locally" by having a concrete class implement it, ie:

public class WeatherForecast implements WeatherConstants {
  // Do stuff with WeatherConstants
}

That a class uses some constants internally is an implementation detail. Implementing a constant interface causes this implementation detail to leak into the class's exported API.

That means, I shouldn't care if a class uses a given constant, so declaring it by implementing an interface is breaking encapsulation.

The book continues:

Worse, it represents a commitment: if in a future release the class is modified so that no longer needs to use the constants, it still must implement the interface to ensure binary compatibility. If a nonfinal class implements a constant interface, all of its subclasses will have their namespaces polluted by the constants in the interface.

There is nothing in the code preventing you from doing this. Just as nothing prevents developers from doing:

new WeatherConstants() { }.SUNNY;

All of these constitute legal code that will compile and run under Java. So, in general, as a developer, you want to shape your APIs to avoid all such nonsense usages. The utility class fixes all of these, as so do the enum where it applies.

[krichter722]

Excellent explanation, thanks a lot! I'd be very happy to see that going into the failure message description (directly to through a link, maybe to the wiki where you could put this).

[dague1]

@all-contributors please add @dague1 for doc

[allcontributors]

@dague1

I've put up a pull request to add @dague1! 🎉

[adangel]

Hi @dague1 , thanks for contributing. Would you mind to create your PR directly on this repo (pmd/pmd), rather than on a fork (Aliasbai/pmd)? -> AliAsbai#9

[dague1]

Hi, @adangel

Sure, I will try to do that!