> how we are going to enforce that _at scale_
Without digging into our infrastructure, I'd assume rolling a check into our
checkstyle targets would cover it. Have different for regular and test so could
have two different rules.
> Now, when I want to know type information my eyes have to jump all over the
> place. This is much worse in longer methods. Reading code is about heuristics
> for where information is, and your brain executing those swiftly for you,
> unconsciously.
There's definitely truth in what you're saying here. I also think there's truth
in that having redundant type information scattered throughout code introduces
a different kind of parsing friction (unconscious heuristic of dismissing
redundant information).
In practice I don't have any measure to compare which is more of a burden; it
probably comes down to what people are more accustomed to. I've certainly
gotten more comfortable with the style in our codebase the more I interact with
it so the status quo of verbosity with light redundancy isn't unbearable by any
stretch. Add in that type inference subtly shifts whether you're binding to an
abstraction or a concrete implementation, and it nudges my opinion against it
in non-test code further.
One other use-case called out in some of the guides / conversations / etc is in
the iteration case. So translating:
for (Map.Entry<String, String> entry : mdc.entrySet())
to
for (var entry : mdc.entrySet())
So much like the idiom of using i in a numeric for loop, or lambda's being
all-in on type inference, I see there being reasonable arguments for usage of
it in very limited scope scenarios. All of which is a lot of work without much
benefit so a simple on/off for prod/test code wfm. /shrug
On Tue, Oct 29, 2024, at 3:13 PM, Štefan Miklošovič wrote:
> I get that there might be some situations when its usage does not pose any
> imminent problem but the question is how we are going to enforce that _at
> scale_? What is allowed and what not ... Sure we can have a code style for
> that, but its existence itself does not guarantee that everybody will adhere
> to that _all the time_. People are people and make mistakes, stuff is
> overlooked etc.
>
> Upon the review we will argue whether "this particular usage of var is
> meaningful or not" and "if the codestyle counts with this or not" and as many
> reviews we will have there will be so many outcomes. Dozens of people
> contribute to the code and more than that reads it, everybody has some
> opinion about that ... Sure, just use var when you prototype etc but I don't
> think it is a lot to ask to merge it without vars. Come on ...
>
> On Tue, Oct 29, 2024 at 8:08 PM Yifan Cai <yc25c...@gmail.com> wrote:
>> I am in favor of *disallowing* the `var` keyword.
>>
>> It does not provide a good readability, especially in the environments w/o
>> type inference, e.g. text editor or github site.
>>
>> It could introduce performance degradation without being noticed. Consider
>> the following code for example,
>>
>>> *Set*<String> allNames()
>>> {
>>> *return null*;
>>> }
>>>
>>> *boolean *contains(String name)
>>> {
>>> *var *names = allNames();
>>> *return *names.contains(name);
>>> }
>> Then, allNames is refactored to return List later. The contains method then
>> runs slower.
>>> *List*<String> allNames()
>>> {
>>> *return null*;
>>> }
>>
>> - Yifan
>>
>> On Tue, Oct 29, 2024 at 11:53 AM Josh McKenzie <jmcken...@apache.org> wrote:
>>> __
>>> (sorry for the double-post)
>>>
>>> Jeremy Hanna kicked this link to a style guideline re: inference my way.
>>> Interesting read for those that are curious:
>>> https://openjdk.org/projects/amber/guides/lvti-style-guide
>>>
>>> On Tue, Oct 29, 2024, at 2:47 PM, Josh McKenzie wrote:
>>>> To illustrate my position from above:
>>>>
>>>> Good usage:
>>>>> Collection<String> names = new ArrayList<>();
>>>> becomes
>>>>> var names = new ArrayList<String>();
>>>>
>>>> Bad usage:
>>>>> Collection<String> names = myObject.getNames();
>>>> becomes
>>>>> var names = myObject.getNames();
>>>>
>>>> Effectively, anything that's not clearly redundant in assignment shouldn't
>>>> use inference IMO. Thinking more deeply on this as well, I think part of
>>>> what I haven't loved is the effective splitting of type information when
>>>> constructing generics:
>>>>
>>>>> Map<InetAddressAndPort, RequestFailureReason> failureReasonsbyEndpoint =
>>>>> new ConcurrentHashMap<>();
>>>> vs.
>>>>> var failureReasonsByEndpoint = new ConcurrentHashMap<InetAddressAndPort,
>>>>> RequestFailureReason>();
>>>>
>>>> I strongly agree that we should optimize for readability, and I think
>>>> using type inference to the extreme of every case where it's allowable
>>>> would be the opposite of that. That said, I do believe there's cases where
>>>> judicious use of type inference make a codebase *more* readable rather
>>>> than less.
>>>>
>>>> All that said, accommodating nuance is hard when it comes to style
>>>> guidelines. A clean simple policy of "don't use type inference outside of
>>>> testing code" is probably more likely to hold up over time for us than
>>>> having more nuanced guidelines.
>>>>
>>>> On Tue, Oct 29, 2024, at 2:19 PM, Štefan Miklošovič wrote:
>>>>> Yes, for now it is pretty much just in SAI. I wanted to know if this is a
>>>>> thing from now on or where we are at with that ...
>>>>>
>>>>> I am afraid that if we don't make this "right" then we will end up with a
>>>>> codebase with inconsistent usage of that and it will be even worse to
>>>>> navigate in it in the long term.
>>>>>
>>>>> I would either ban its usage or allow it only in strictly enumerated
>>>>> situations. However, that is just hard to check upon reviews with 100%
>>>>> accuracy and I don't think there is some "checker" to check allowed
>>>>> usages for us. That being said and to be on the safe side of things I
>>>>> would just ban it completely.
>>>>>
>>>>> Sometimes I am just reading the code from GitHub and it might be also
>>>>> tricky to review PRs. Not absolutely every PR is reviewed in IDE, some
>>>>> reviews are given without automatically checking it in IDE too and it
>>>>> would just make life harder for reviewers if they had to figure out what
>>>>> the types are etc ...
>>>>>
>>>>> On Tue, Oct 29, 2024 at 7:10 PM Brandon Williams <dri...@gmail.com> wrote:
>>>>>> On Tue, Oct 29, 2024 at 12:15 PM Štefan Miklošovič
>>>>>> <smikloso...@apache.org> wrote:
>>>>>> > I think this is a new concept here which was introduced recently with
>>>>>> > support of Java 11 / Java 17 after we dropped 8.
>>>>>>
>>>>>> To put a finer point on that, 4.1 has 3 hits, none of which are valid,
>>>>>> while 5.0 has 172. If 'sai' is added to the 5.0 grep, 85% of them are
>>>>>> retained.
>>>>>>
>>>>>> Kind Regards,
>>>>>> Brandon
>>>>
>>>
