PAN-2592: Rename methods that create and return streams away from getX()

[shemnon]

PR description

Change all Stream getX() methods to Stream streamX() methods

[ajsutton]

This may be one for a slightly broader audience. Personally I would have used get for this and have no expectation that any two calls to a get method would return the same instance (I view getters as a deliberate abstraction to avoid callers knowing how the class stores or calculates the value). But I'm not particularly fussed and can see the argument for going the other way.

Ultimately we'll want to be consistent with how we do this so should make a bit of noise about it and probably update the code style doc.

[Errorific]

I disagree with this. After talking with @shemnon the logic behind this is that getters return references to properties. I feel similarly to @ajsutton that getters (in fact all public methods) should be an abstraction that hides the implementation details.

Exposing direct access to properties through getters and expecting it to be direct access makes the code more tightly coupled.
Making data access of an object use 2 different grammars (get and no get) gets more confusing when reading the code.

[shemnon]

What then is the difference between a property that uses a getX pattern and any other zero argument method? Should we make all zero argument methods getters? Making everything a property would be an impediment to understanding because get then becomes a marker for a zero argument method. There should be some standard to delineate methods from properties. What would the corresponding setter be? In this case it would be the base collection that the stream is derived off of, so the getter should similarly be the collection, and not an operation on the collection.

Methods that encapsulate operations on the underlying property are just that, methods. By exposing a stream method instead of a property and letting the user stream the property we gain some encapsulation by not exposing the mutable collection that the stream is derived from. But the Stream<?> getX methods are not the properties, they are hidden transformations on the properties.

The Java standards on properties provides zero guidance as to the difference between a getter and a no-args method. Google's internal standards do, and align with this, but they are not available externally.

Microsoft has properties vs method guidance (https://docs.microsoft.com/en-us/previous-versions/dotnet/netframework-4.0/ms229054(v=vs.100)#section-5). The most relevant two in this case are

• "The operation is a conversion, such as the Object.ToString method." - collection.stream() and collection.iterator() are conversions.
• "The operation returns a copy of an internal state (this does not include copies of value type objects returned on the stack)." - an iterator and a stream are a copy of the state of the property streamed or iterated

[Errorific]

Making all zero argument methods that don't perform mutations a getter sounds at least consistent.

I don't see how it becomes an impediment to understanding. If we suggest that a getter retrieves a value without causing a change to the object you're requesting it from then that's straightforward to understand. Maybe we need to talk more about how you're defining methods and properties.

Being able to define a corresponding setter doesn't really factor into whether it should be prefixed get or not, there is plenty of scope for read only values that don't have setters. If the example is logically a collection that's being represented as a stream then perhaps we need List<> getValue(), setValue(List<>), and Stream<> getValueStream().

For the relevant examples from Microsoft:

• The operation isn't a conversion, if you were doing Object.getList().stream() then it would be, in this case it's Object.getStreamOfList() which isn't converting the object and you shouldn't have any visibility or concern of whether it's converting something internal to the object.
• I think you're misinterpreting the second one, the example CopyEmployeeRecords() leads me to think they're referring to copies of the entire objects state. Whether a copy is being returned or not shouldn't impact the consumer and I think it's a bad idea to assume you have a volatile reference to internal object state just because you used a getter.

I think more relevant points from the Microsoft literature around when a method should be used include:

• The operation has a significant and observable side effect. Note that populating an internal cache is not generally considered an observable side effect.
• The operation returns a different result each time it is called, even if the parameters do not change. For example, the NewGuid method returns a different value each time it is called.

[shemnon]

Perhaps we should rephrase the standard to prefer method names unless the method represents the getting or setting of a property. Putting the preference to methods so we don't get into strange cases where "it could be seen as a property" like I feel is going on here.

In the Microsoft standards there is a section before providing a positive test for properties over methods: "Do use a property, rather than a method, if the value of the property is stored in the process memory and the property would just provide access to the value." - we are not returning the value of the field but instead an operation on the value of that field. By that standard this is not a property.

From the "Not a property" list the second and fifth standard fail, and the third mostly fails.

• speed is not an issue, it's all local data.
• The property is the List. The stream is a view onto that property. The call to stream() is the transformation to that view. Assigning the getter to the stream from that list obscures that fact. Simply calling it streamOfList better communicates this.
• The copy in some cases (when the List can be a CopyOnWriteArray) is what the list contains. But in all cases there is new transient data as to where your cursor is within that list.
• No real side effects outside the returned object, so this standard does not apply.
• Each call to stream or iterator returns a different object. Not just because .equals() fails (it's not a value object) but also because the transient state differs between the two. If you create a stream twice, and then stream all the data out of the first one then the second one is still ready to be streamed, untouched.
• A List or a Stream is not an array, even when the backing store in the ArrayList is an array.

I've tried searching to see what other projects do in this case. After filtering out I/O Streams all the cases I've seen just return the list and have the user stream off that object. So if we were to follow industry practice we would actually replace the method with one that returns a List or Collection and let the caller stream or iterator at their preference.

[ajsutton]

Couple of things:

1. We probably shouldn't be putting any weight on Microsoft's coding style given that it's .NET focussed and Java uses quite a different approach.

2. More importantly, we should take a step back and ask what the cost/benefit of either using or not using the get prefix in any of these particular cases is.

I'm struggling to see a discernible impact on productivity or code quality so long as the method name makes sense.

[shemnon]

The reason I used a Microsoft standard is because I cannot find any Java based guidance on when to use a getter/setter and when to use a non-property method name. The closest I can find is one that says only use it for returning fields, a very unsatisfying standard.

The concern I have is we have a case where the standard is essentially "what is your opinion" rather than an objective standard. This leads to a case such as this where one developer has one opinion, and another developer is blocking approval based on their opinion. That's why I went to dot net, which was heavily influenced by java. Actually I'de love to use Google's recommendations for Java readability, but those are not available outside the chocolate factory.

So this creates a governance issue, where one developer has blocked the submission of a PR based on their opinion where the authors feels strongly about their opinion. One opinion has to lose, or it needs to be removed from the realm of opinion.

[shemnon]

After a zoom call the consensus was

• limit this to just no-args methods returning stream, leave Iterators out of it.
• change the naming convention to streamSomething such as streamIdlePeers