Public fields passed through by VisibilityModifierCheck

[Marko Topolnik]

I cannot understand the rationale behind allowing public final fields only in final classes. What is the difference if the class is not final? The field canot be overridden. 

I often expose final fields from my class without burdening the design with extra getters and I don't see any potential for harm.

--

Marko Topolnik

Hazelcast

[Roman Ivanov]

Hi Marko,

thanks a lot for attention, and bringing that issue back to attention.

I do not remember a reason :( .

But I know that I had the same question - https://github.com/checkstyle/checkstyle/issues/61#issuecomment-74566597

But for some reason I missed that during implementation or found an explanation/reason and did not put it to issue.

I will think a bit more about this and will ping involved engineers to explain that.

thanks,

Roman Ivanov

​

[Marko Topolnik]

From what I've read on the github issue, your argument offering a specific case where public final fields are useful---immutable classes---got confused for a restriction on what the rule should apply to. Immutable class, among many other constraints, must be final because a subclass might introduce mutabilty. 

However, neither is it relevant to this rule whether the class is immutable, nor does the rule actually check for class immutability. So my conclusion is that this requirement resulted from a miscommunication.

Regards,

Marko Topolnik

[Marko Topolnik]

...but I would also argue that the essence of this issue is not at all about final. A final vs. nonfinal field is the same as getter vs. getter+setter. The real essence is something completely different: if your design calls for a simple accessor with no more logic than read/write-through, that fact itself implies that you are forfeiting 99% of encapsulation. Real encapsulation happens when a class provides an abstraction over some internal state composed of more than one field. The fields are mutually dependent and encapsulation ensures they change in a coordinated manner (maintaining the class's invariants).

The 1% encapsulation that such accessors provide is that they may or may not read/write directly to a single, same-typed field. This choice becomes implementation detail and can change in a later version, or in a subclass, without impacting the client. In practice this liberty is almost never exercised. If you are in control of the client code, you can always introduce accessors later on, when a real need for them arises. So about the only place where you must ensure this liberty in advance is public API. Even in public library projects, classes which constitute the actual public API figure with less than 1% in the total class count on a project (public API is mostly interfaces, anyway).

In conclusion, a class should either be allowed or disallowed to declare public fields---whether final or not. Applying this line of reasoning to VisibilityModifierCheck, it seems that it makes the entire exception given to final fields moot.

--

Marko

[Roman Ivanov]

Hi Marko,

Sorry for delay, i will reply to your during that week.

Thanks.
Roman Ivanov

[Roman Ivanov]

Hi Marko,

Sorry for late reply, too much is mixed in that topic that was hard to clear out what we are talking about.

> I cannot understand the rationale behind allowing public final fields only in final classes. What is the difference if the class is not final? The field canot be overridden. 

Agree, but that is just additional security for case described there - https://github.com/checkstyle/checkstyle/issues/61#issuecomment-109317543

> I often expose final fields from my class without burdening the design with extra getters and I don't see any potential for harm.

http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier

Agree to some extend, and we try to make it possible by option, and let user decide what he need to check.

 > The real essence is something completely different: if your design calls for a simple accessor with no more logic than read/write-through, that fact itself implies that you are forfeiting 99% of encapsulation. ..... 

....

Even in public library projects, classes which constitute the actual public API figure with less than 1%

Agree, stupid usage of get+set is ...... , but we are not in position to tell user how he need to design his class. We work from a position that there is such tendency of code appearance and we control it.  People want that we give then that option.

You can find that some Check as conflicting as demand opposite - only user could decide what Check and in what configuration should be used on its own code. We just give tool to user. Misuse is always possible.

> In conclusion, a class should either be allowed or disallowed to declare public fields---whether final or not.

You are absolutely right ! So you have option to use that Check or not. 

Checkstyle is tool set and you cherry pick individual tools you need or want or  care about. You adjust tools to your needs (defaults values might not work well and should be changed to match reality of your project).

 

Checkstyle is not tool like Findbugs, where all violations are bugs and has to be fixed. Checkstyle is more like an high level formatter for Java - we highlight some potentially bad code or non project up-to-standard or inner team agreements - but it could work very well in both variants (before and after fix).

thanks,

Roman Ivanov

[Marko Topolnik]

On 09 Jul 2015, at 05:07, Roman Ivanov <romani...@gmail.com> wrote:

> I cannot understand the rationale behind allowing public final fields only in final classes. What is the difference if the class is not final? The field canot be overridden. 

Agree, but that is just additional security for case described there - https://github.com/checkstyle/checkstyle/issues/61#issuecomment-109317543

This mixes up the topic of individual final fields with that of immutable classes. Public fields are just as useful in immutable as in mutable classes, and for exactly the same reasons. The arguments presented just perpetuate the truism that code can be written which satisfies the formalities yet breaks common sense. If this wasn't true, the implication would be that CheckStyle has become an entitiy of human-level intelligence, capable of writing impeccable code on its own.

> I often expose final fields from my class without burdening the design with extra getters and I don't see any potential for harm.

http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier

Agree to some extend, and we try to make it possible by option, and let user decide what he need to check.

Quotes from the VisibilityModifier page:

Rationale: Forcing all fields of class to have private modified by default is good in most cases, but in some cases it drawbacks in too much boilerplate get/set code. One of such cases are immutable classes.

Restriction: Check doesn't check if class is immutable, there's no checking if accessory methods are missing and all fields are immutable, we only check if current field is immutable and defined in final class

Taken as a whole, they don't paint a very consistent picture. The check apparently tries to be opinionated and allow public final fields only for immutable classes, but doesn't really check that the class is immutable; on the other hand it does nag the programmer about the class being final. Being opinionated is generally not the CheckStyle way: let the user choose. This whole config option seems to be an outgrowth from one specific request, meeting that requestor's specific wishes. It doesn't generalize well.

> In conclusion, a class should either be allowed or disallowed to declare public fields---whether final or not.

You are absolutely right ! So you have option to use that Check or not. 

Checkstyle is tool set and you cherry pick individual tools you need or want or  care about. You adjust tools to your needs (defaults values might not work well and should be changed to match reality of your project).

This is how it all started... I saw that allowPublicFinalFields was true even by default, yet my public final field was being flagged as a violation of encapsulation.

---

Marko

[Roman Ivanov]

Hi Marko, 

> Being opinionated is generally not the CheckStyle way: let the user choose.

:) yes, super users choose how to make config , all other follow.

> This is how it all started... I saw that allowPublicFinalFields was true even by default, yet my public final field was being flagged as a violation of encapsulation.

I really want to finish that discussion , but I do not have that much time, too busy on the project. And I still blindly do not see a problem you point to.

To speed up a process, lets start speaking by code examples+config before questionable option and after.

Please provide me such examples as much as reasonable, that will help me a lot.

thanks in advance,

Roman Ivanov

​

[Marko Topolnik]

My point is quite simple: the option to allow public final fields should do so without further ado. The rest of the talk here was an attempt to show why the additonal restriction to final class is arbitrary and reduces usefulness of the option.

Example:

public class Chunk {

  public final long id;

  private int garbage; 

  ....

}

public class StableChunk extends Chunk {

  // inherits and uses id

  private final int size;

  ...

}

public class GrowingChunk extends Chunk {

  // inherits and uses id

  private int size;

  ...

}

In the above example the class Chunk is essentially non-final yet makes perfect use of a public final field. CheckStyle's current option does not allow me to codify this.

---

Marko

[Roman Ivanov]

Hi Marko,

sorry for delay.

thanks a lot for example , it helped.

Looks like reason of misunderstanding it a Check functionality - it try to check too much. Our bad design.

http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier

Example that you provided is valid to some extend. It is matter of style: to have public final fields or wrap them with get/set .

So, I propose to have new property "allowPublicFinal"="false/true" ("false" is default.).

That will reconcile two groups of engineers on how to manage access to fields. 

Please confirm that this will resolve your problem and create issue against us on github - https://github.com/checkstyle/checkstyle/issues .

Or just reply to mail-list and I will register it.

FYI: http://checkstyle.sourceforge.net/report_issue.html#How_to_report_a_bug

Sorry again for long delay in reply.

thanks,

Roman Ivanov

[Marko Topolnik]

On Tuesday, February 23, 2016 at 10:10:48 PM UTC+1, Roman Ivanov wrote:

Hi Marko,

sorry for delay.

thanks a lot for example , it helped.

Looks like reason of misunderstanding it a Check functionality - it try to check too much. Our bad design.

http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier

Example that you provided is valid to some extend. It is matter of style: to have public final fields or wrap them with get/set .

So, I propose to have new property "allowPublicFinal"="false/true" ("false" is default.).

That will reconcile two groups of engineers on how to manage access to fields. 

I guess the actual name would be "allowPublicFinalFields". It would be enough for me, but confusingly similar to the existing "allowPublicImmutableFields". I will let the CheckStyle maintainers resolve this as they see fit.

 

Please confirm that this will resolve your problem and create issue against us on github - https://github.com/checkstyle/checkstyle/issues .

I will create an issue.

Best,

Marko