[DISCUSS] IntelliJ Code Analysis + Code Style

[Mike Drob <md...@apache.org>]

Hi Devs,

I recently ran the IntelliJ code analysis on the hbase project and saw that
it had some interesting suggestions.

A lot of them don't point to bugs, but really end up being more of a
stylistic issues. The ones I want to discuss specifically are the ones it
categorizes as java version issues. Here's the list:

Java 5:
* 'for' loop replaceable with 'foreach'
* Unnecessary unboxing
Java 7:
* Explicit type can be replaced with <>
* Identical 'catch' branch in 'try' statements
* Possible heap pollution from vararg type
Java 8:
* Anonymous type can be replaced with lambda
* Anonymous type can be replaced with method reference
* Lambda can be replaced with method reference
* Statement lambda can be replaced with expression lambda

I think I read somewhere once that 'foreach' is not great for garbage
collection because it creates an extra iterator, so we can maybe ignore
that one. And I personally prefer statement lambdas (with curly braces) to
expression lambdas (without) especially for long lines, so ignore that one
too. But maybe address the rest?

If we focus on only the Java 5/7 issues, then we can apply them to both
branch-1 and branch-2. Not sure how much value that provides, as I don't
have the historical context to estimate how long branch-1 will continue to
live once branch-2 is out.

The IDE can do the code changes automatically, but it will still be a lot
of time to review, hence the discussion before I cavalierly go and file an
issue and attach a patch. Also, I have no idea how to turn these into
automated checks once we fix any of them, and that seems kind of important.

IntelliJ has other checks that we can look at as well, but these were the
ones that caught my eye first as easiest to fix and least likely to false
positive.

Thoughts?

Mike

Re: [DISCUSS] IntelliJ Code Analysis + Code Style

[Mike Drob <md...@apache.org>]

I don't have a hard rule about it.

Maybe things that affect block indentation are too much change. One line
changes are probably ok, like replacing explicit types with the diamond
operator.

Consolidating catch blocks is probably good because it makes the code more
clear and easier to maintain/understand in the future.

Now that I've started writing this out, I think that's a good metric - when
it makes the code more readable and understandable then it's a good idea,
regardless of number of lines changed.

When the Java 8 was young and lambdas and method references were this brand
new thing, then maybe they were less readable because they were unfamiliar
and developers were uncertain how to treat them, Now that we're come years
in, I wonder if developers are more fluent in these new language features.

By this logic, the Java 5 and 7 changes are almost certainly good
candidates for inclusion since they've been around for a very long time
now. Is the community familiar enough with how to read a lambda expression?
It took me a while to understand them well, and now I find them quite
clear, but I won't claim that my experience is universal.

Food for thought,


Mike

On Mon, Jun 19, 2017 at 4:40 PM, Ted Yu <yu...@gmail.com> wrote:

> bq. we can apply them to both branch-1 and branch-2
>
> Many bug fixes are still targeting 1.x releases. If the stylistic fixes go
> to branch-1, would that make porting between 1.x releases and branch-1 more
> cumbersome ?
>
> What is the amount of changes for the stylistic fixes you deem applicable ?
>
> Cheers
>
> On Mon, Jun 19, 2017 at 2:30 PM, Mike Drob <md...@apache.org> wrote:
>
> > Hi Devs,
> >
> > I recently ran the IntelliJ code analysis on the hbase project and saw
> that
> > it had some interesting suggestions.
> >
> > A lot of them don't point to bugs, but really end up being more of a
> > stylistic issues. The ones I want to discuss specifically are the ones it
> > categorizes as java version issues. Here's the list:
> >
> > Java 5:
> > * 'for' loop replaceable with 'foreach'
> > * Unnecessary unboxing
> > Java 7:
> > * Explicit type can be replaced with <>
> > * Identical 'catch' branch in 'try' statements
> > * Possible heap pollution from vararg type
> > Java 8:
> > * Anonymous type can be replaced with lambda
> > * Anonymous type can be replaced with method reference
> > * Lambda can be replaced with method reference
> > * Statement lambda can be replaced with expression lambda
> >
> > I think I read somewhere once that 'foreach' is not great for garbage
> > collection because it creates an extra iterator, so we can maybe ignore
> > that one. And I personally prefer statement lambdas (with curly braces)
> to
> > expression lambdas (without) especially for long lines, so ignore that
> one
> > too. But maybe address the rest?
> >
> > If we focus on only the Java 5/7 issues, then we can apply them to both
> > branch-1 and branch-2. Not sure how much value that provides, as I don't
> > have the historical context to estimate how long branch-1 will continue
> to
> > live once branch-2 is out.
> >
> > The IDE can do the code changes automatically, but it will still be a lot
> > of time to review, hence the discussion before I cavalierly go and file
> an
> > issue and attach a patch. Also, I have no idea how to turn these into
> > automated checks once we fix any of them, and that seems kind of
> important.
> >
> > IntelliJ has other checks that we can look at as well, but these were the
> > ones that caught my eye first as easiest to fix and least likely to false
> > positive.
> >
> > Thoughts?
> >
> > Mike
> >
>

Re: [DISCUSS] IntelliJ Code Analysis + Code Style

[Ted Yu <yu...@gmail.com>]

bq. we can apply them to both branch-1 and branch-2

Many bug fixes are still targeting 1.x releases. If the stylistic fixes go
to branch-1, would that make porting between 1.x releases and branch-1 more
cumbersome ?

What is the amount of changes for the stylistic fixes you deem applicable ?

Cheers

On Mon, Jun 19, 2017 at 2:30 PM, Mike Drob <md...@apache.org> wrote:

> Hi Devs,
>
> I recently ran the IntelliJ code analysis on the hbase project and saw that
> it had some interesting suggestions.
>
> A lot of them don't point to bugs, but really end up being more of a
> stylistic issues. The ones I want to discuss specifically are the ones it
> categorizes as java version issues. Here's the list:
>
> Java 5:
> * 'for' loop replaceable with 'foreach'
> * Unnecessary unboxing
> Java 7:
> * Explicit type can be replaced with <>
> * Identical 'catch' branch in 'try' statements
> * Possible heap pollution from vararg type
> Java 8:
> * Anonymous type can be replaced with lambda
> * Anonymous type can be replaced with method reference
> * Lambda can be replaced with method reference
> * Statement lambda can be replaced with expression lambda
>
> I think I read somewhere once that 'foreach' is not great for garbage
> collection because it creates an extra iterator, so we can maybe ignore
> that one. And I personally prefer statement lambdas (with curly braces) to
> expression lambdas (without) especially for long lines, so ignore that one
> too. But maybe address the rest?
>
> If we focus on only the Java 5/7 issues, then we can apply them to both
> branch-1 and branch-2. Not sure how much value that provides, as I don't
> have the historical context to estimate how long branch-1 will continue to
> live once branch-2 is out.
>
> The IDE can do the code changes automatically, but it will still be a lot
> of time to review, hence the discussion before I cavalierly go and file an
> issue and attach a patch. Also, I have no idea how to turn these into
> automated checks once we fix any of them, and that seems kind of important.
>
> IntelliJ has other checks that we can look at as well, but these were the
> ones that caught my eye first as easiest to fix and least likely to false
> positive.
>
> Thoughts?
>
> Mike
>