1

Some static analysis tools flag non-private fields with

Variable '[nameHere]' must be private and have accessor methods.

Sonar consistently presents such warnings and wants to change all protected variables in my code to private, why is this? I'm yet to see a protected variable in my work that hasn't raised this warning.

Reginald
  • 113
  • 1
  • 4
  • 3
    There is [a question on Stack Overflow about this same warning in Checkstyle](http://stackoverflow.com/questions/8465450/why-are-protected-variables-not-allowed-by-default-in-checkstyle/). The accepted answer links to [a page in the Checkstyle documentation that explains rational behind this warning](http://checkstyle.sourceforge.net/config_design.html). – Thomas Owens Mar 12 '15 at 11:54

2 Answers2

1

Fields that are protected are not hidden strongly enough for many static analysis tools such as sonar, pmd, or checkstyle. protected fields are visible to subclasses (docs) and allow subclasses to change the state of the class without going through accessor methods which may cause problems in the methods of the superclass.

So, that's why its complaining about protected fields. Unless you are specifically designing for extension (that the class should be subclassed), it is probably better to use the default package-private level for fields and methods that you want visible to tests (or other classes in the package).

Personally, I've never had use for a protected variable field within a class - even those that are subclassed. protected final fields occasionally, but never a variable field. On the other hand, I've often used default level on methods for testing and (more often than protected final though still uncommon) /* default */ final fields.

The purpose of the warning is to get you to think more about how you are actually presenting the class and the contract you are presenting to others for its use. If you have it be too permissive, it is all too easy for co-workers to take short cuts and tinker with the innards of the class (I once saw someone create an inner class that subclassed another class and create public methods to access the protected fields). Reducing the visibility of fields and methods can be a painful refactoring.

Start out with the most restrictive permissions and as you find real need for a design that is more permissive, open it up. This is what the static analysis tools are encouraging you to do.

  • That is just plain wrong. Using the default package protected variation leads just to the same message. So I don't see a way to avoid that without throwing out the child with the water (having no change to test from the inside out). – mliebelt Apr 03 '16 at 14:00
-2

The purpose of private variables is to hide implementation details from the users of the class so that the implementation can be changed without breaking all the code that uses it.

Inheritance creates a second set of "users" of your class - its subclasses. Leaving instance variables protected is equivalent to exposing them as public to the subclasses.

Doval
  • 15,347
  • 3
  • 43
  • 58
  • 2
    That does not explain why this should be automatically banned. – DeadMG Mar 12 '15 at 12:17
  • @DeadMG That's not what the question asks, and whether something should be automatically banned is an entirely different - and subjective - question, which depends on your definitions of "automatically" and "banned". Do you mean *automatically* as in "enforced by a tool" or as in"this should be your default"? Do you mean *banned* as in "unconditionally banned" or as in "banned unless justification is given and reviewed by your peers"? That variables should be private by default is part of most coding standards and that's all the explanation needed for why a static analysis tool would flag it. – Doval Mar 12 '15 at 12:24