Issue #6764: fix some sonar warnings and small refactoring

[strkkk]

Issue #6764

[romani]

We have profile at Sonar, please provide direct link to violation.
Teamcity (TC) is dedicated to IDEA violations (we have config in .ci folder) .... I do not see failures.

Please put all details in PR description of what violations your are try to resolve.
Ones you have all details in PR, please reference PR numbers in git commit like Pull #XXXXX: .......

[strkkk]

@romani done.
about IDEA things - it is not failures, just warnings that code can be simplified, you see them if you open code in IDE.
I changed two methods to use optional in more functional style. If it is not needed, I can revert this changes.
Original commit had deletion of empty finishTree method, but I brought it back after explanation in review

[rnveach]

@strkkk since you made an issue for this it should be Issue #6764. If you didn't create an issue it would be Pull #6760.
I only mentioned an issue since you were changing the check because of removing the final method.
Sorry for any confusion.

[strkkk]

@rnveach sorry, I understood romani's comment in a wrong way.
Will reword commit message.

[strkkk]

@rnveach can you clarify - codecov checks failed because of travis build (which failed because of unapproved issue)?
I see that coverage is the same (100%)

[rnveach]

@strkkk I think codecov is just a random failure. I don't remember seeing it connected to travis in such a way before.
travis failure is because issue isn't approved and am waiting on @romani to review it.

[romani]

about IDEA things - it is not failures, just warnings that code can be simplified

Whole idea of TC is allow us to use the most of inspection set.
User should open project in Idea , configure it by https://checkstyle.org/idea.html#Inspections and have no warnings and errors. Role of TC is control it.

I am fine to make code better, your change is good.

But I want to get to know , what inspection is active on your side and disabled on our?

[romani]

Item to discuss:

[romani]

I might be old-school, but I like to have simple return statement, so it is easy to put break point on return and validate result. Extra local variable is super easy task for any optimizer, but it improve code maintenance.

@rnveach , what is your position on this ?

[rnveach]

I am even more old school then you. I am not a fan of lambdas and the new streams in Java 8 because they hide some functionality and some of them make the code harder to read, imo. I maintain the backport and these always give me issues converting since I have to understand them to convert them. Understanding them never comes instant for me even though I have converted a bunch.

Can't say I have really tried to put a breakpoint here but I tried and Eclipse Oxygen won't let me put it on the key -> line unless there was {}s. Didn't try newer eclipse versions.
New code is still easy to read. We have alot of other areas I would choose to remove lambdas first over starting with this.
I am pretty much fine with this one.

[romani]

@strkkk ,

Please do me favor, keep return javadocTree.

[strkkk]

@romani Ok, I've brought this one back.

[strkkk]

@romani this particular inspection is Optional.isPresent() can be replaced with functional-style expression
It looks like I forgot to import inspections for checkstyle in this machine (I have a few machines to work from) and this issue was detected.

[romani]

Look at our config, we probably need to enable it, if it doesn't have special disablement comment.

[strkkk]

@romani

Look at our config, we probably need to enable it, if it doesn't have special disablement comment.

It is disabled by default -

checkstyle/config/intellij-idea-inspections.xml

Lines 3418 to 3422 in 141f5d6

	<!-- on the moment of review we were not a big fans of functional style,
	none of code coverage tools can distinguish that body of lambda is executed.
	When this problem is resolved we could reconsider disablement. -->
	<inspection_tool class="OptionalIsPresent" enabled="false" level="WARNING"
	enabled_by_default="false"/>

I have no idea is this problem solved or not.

[romani]

Good. I didn't find in related PR or issue any details, but most likely this comment was included by me.
Reason is that we are benefit a lot from 100% coverage by simple code coverage (jacoco) and mutation test(pitest).
In non functional code style, such tool will demand to provide more test inputs, so they definitely contribute to quality. In functional style, they will not alarm, human for definitely miss incomplete tests.
I do not see mutation in lamda http://pitest.org/quickstart/mutators/ .

So we are are not crazy about single statement style (functional), for good reason. But in same time do not want to be old code, so we do not force functional style automatically, and see all case by case.

[rnveach]

I do not see mutation in lamda

Pitest mutates the bytecode. There is no lambda in the byte code, so that is why there is no specific mutator for them.
Example Code:

void printElements(List<String> strings){
    strings.forEach(item -> System.out.println("Item = %s", item));
}

Decompiled Bytecode:

private static CallSite cs;

void printElements(List < String > strings) {
    Consumer < String > lambda;

    //begin invokedynamic
    if (cs == null)
        cs = bootstrapLambda(MethodHandles.lookup(), "lambda_forEach", MethodType.methodType(void.class, String.class));

    lambda = (Consumer < String > ) cs.getTarget().invokeExact();
    //end invokedynamic

    strings.forEach(lambda);
}

private static void lambda_forEach(String item) { //generated by Java compiler
    System.out.println("Item = %s", item);
}

private static CallSite bootstrapLambda(Lookup lookup, String name, MethodType type) { //
    //lookup = provided by VM
    //name = "lambda_forEach", provided by VM
    //type = String -> void

    MethodHandle lambdaImplementation = lookup.findStatic(lookup.lookupClass(), name, type);

    return LambdaMetafactory.metafactory(lookup,
        "accept",
        MethodType.methodType(Consumer.class), //signature of lambda factory
        MethodType.methodType(void.class, Object.class), //signature of method Consumer.accept after type erasure  
        lambdaImplementation, //reference to method with lambda body
        type);
}

https://dzone.com/articles/hacking-lambda-expressions-in-java