Change wording of rule to consider READ-ONLY attributes

[ConjuringCoffee]

Our discussion in #299 brought to light that "immutability" is a word that shouldn't be used lightly.

In the case of READ-ONLY attributes, these variables definitely aren't immutable: The variable can be changed from within the class, just not from outside. I have therefore removed the word "immutable" from the rule.

[ConjuringCoffee]

The build fails due to dead links which is unrelated to this change. I have fixed the links in #328.

[kjetil-kilhavn]

Dead links to abapGit documentation causes the build to fail, fixed in #329

[ConjuringCoffee]

Interesting! I think I misunderstood the intention of the original chapter. I rephrased the rule again to hopefully get the original intention across.

I've turned the pull request into a draft because there's one thing left that I'd like to get some feedback on. Regarding this:

Considering that it can only be used in a PUBLIC SECTION and public access should always be goverened by interfaces, though, I would hope to not encounter it very often in a clean and easily testable codebase.

Should we address how interfaces should be considered with this section? Interfaces can define attributes (including READ-ONLY attributes) too. It can be argued that the section currently proposes putting READ-ONLY attributes into interfaces. What do you think?

(Personally, I think interfaces should only define behavior and shouldn't contain any attributes, but some of my colleagues disagree. I don't think that Clean ABAP defines anything in this regard yet.)

[bjoern-jueliger-sap]

A READ-ONLY attribute in an interface to me is the same behavior specification as having a getter method, with the additional constraint that the result of the getter must not be computed at run time. This can make sense in application where getting this value from the interface is performance critical and you want to prevent someone messing up the performance of your application by (intentionally or unintentionally) writing an expensive implementation for the getter.

That is, the rule for using READ-ONLY in classes and interfaces should be exactly the same: Use it if and only if a getter method is unacceptable for performance reasons.

[pokrakam]

Interesting discussion, but I feel that immutability and writability should not be mixed up as they are here, they're very different ideas. READ-ONLY only applies to external consumers. If we do need to expose an attribute as PUBLIC then it is recommended to make it READ-ONLY for stability reasons. As Bjoern already said, performance is one reason we may want to make an attribute public for the odd critical bottleneck. There are some technical scenarios too, e.g. use in Sapscript, or components that show attributes (common in workflow), there are some mapping features in various places that let you connect a class attribute to things. I think all these scenarios are what READ-ONLY is intended for.

[ConjuringCoffee]

That is, the rule for using READ-ONLY in classes and interfaces should be exactly the same: Use it if and only if a getter method is unacceptable for performance reasons.

Does this mean that you are against the original intention of the section, which seems to be "You can consider using READ-ONLY when working with immutable objects because a getter would be bloat"? If you are against that, then no amount of rephrasing of the original assumed intent will be sufficient.

The more we discuss, the more it becomes clear that it isn't just this section that is unclear. The explanation of "Use READ-ONLY sparingly" is outdated: My understanding of that section is "Do not use READ-ONLY if you think it is a replacement for FINAL", but the wording is confusing because it was written in a time when FINAL didn't exist yet.

Are we really discussing the use of READ-ONLY here, or are we discussing public attributes?

Here's a new suggestion on merging the two sections with the section before, making some information more explicit and putting them in a different order:

• Leading rule: "Members PRIVATE by default, PROTECTED only if needed"
• Same explanation as the rule currently
• Exception 1: "Public attributes are faster than getters. They can be considered if critical for performance reasons"
• Exception 2: "Public attributes can be considered if working with immutable objects because they never change and a getter could be considered bloat."
• For these exceptions, make the attribute READ-ONLY or FINAL if possible.

What do you think?

[bjoern-jueliger-sap]

Caution: final can only be used in inline declarations for local variables, it is not an alternative to read-only, since read-only applies only to public attributes of a class. Every variable that can be made read-only cannot be made final and vice versa.

The "immutable objects" part should perhaps carry a performance hint: Im my tests, creating a bunch of plain data objects is between 4 and 10 times faster than creating the same number of instances of an object that holds the same data as read-only public attributes, and accessing the read-only attributes is 25% slower than accessing the component of the equivalent plain data object. So if these objects are the main kind of object your application processes, then you might find that switching out your data structures for classes with read-only attributes slows down your application.

[ConjuringCoffee]

I've reworked the text again, but ended up with something very close to the original. The original is quite broad, but in turn doesn't need all the nuances introduced when referring to the READ-ONLY or FINAL keywords.

I think it's for the best to close this pull request without any further changes.