Fix Controller.shouldBeCheckedForEnhancement method

[dcrall]

Rename the method Controller.shouldBeCheckedForEnhancement to Controller.enhancementCheckComplete to accurately reflect what the code is doing. Addresses issue #1271.

[asolntsev]

@dcrall I agree that current name shouldBeCheckedForEnhancement is incorrect. It should be the opposite: shouldNotBeCheckedForEnhancement. But your proposed name enhancementCheckComplete doesn't seem to be clear for me. Why "check is complete"? At that moment, enhancement check is not even started yet.

[dcrall]

@asolntsev Thank you for reviewing the change. When the condition in this method is true, we actually return from the method, so the enhancement check is complete. We check for enhancements while we see application code. As soon as we're back in framework code, we know all application code is enhanced and the enhancement check is complete. I believe my method name is correct.

[asolntsev]

@dcrall You are talking about the code that calls method shouldBeCheckedForEnhancement, not abut this method itself. Any method's name should reflect what it does, not how it's used by someone else.

If method does FOO, it should be named FOO, no matter if it's used for BAR or BAR2 or whatever else.

[dcrall]

@asolntsev If your argument is that the method should describe what it is mechanically checking, then the name should be something like isFrameworkClass. However, I believe the "what" is that we are looking to see if our enhancement check is complete. The "how" is that we have re-entered framework code in the stack trace.

Additionally, this is not a general purpose method. This is a method that was extracted to provide clarity in the calling code. It is hard to see how it will be used in any other context. Regardless, the method has nothing to do with if that class should be checked for enhancement.

I think if we want to focus on the enhancement check, which makes sense. I would suggest something like this:

if (haveSeenFirstApplicationClass) {
    if(!isFrameworkClass(className)) {
        // check for enhancement
    } else {
        return; //enchancement check complete
    }
}

This structure complements the haveSeenFirstApplicationClass flag and makes the exit check secondary to the primary work of the method. I would be happy to make this change if you think it better.

[asolntsev]

@dcrall No, isFrameworkClass is not a proper name because jdk.* and sun.* are not part of Play framework.

I agree, this is not a general-purpose method, and it's not intended to be used from outside.
But still, this design principal is general, independent from method scope: method name should reflect what id does.

I don't think it should be paired with haveSeenFirstApplicationClass, I think it should be paired with ContinuationEnhancer.isEnhanced(className). Because depending on the result of our method we decide if we should call isEnhanced. So, it seems reasonable flow to me:

if (should be check for enhancement) {
  <check for enhancement>
}

[dcrall]

@asolntsev - Okay, this is my last attempt. If you don't like this, go ahead and change the name to shouldNotBeCheckedForEnhancement() and be done with it.

My change is really two fold at this point. One, I changed haveSeenFirstApplicationClass to verificationStarted. This marks the first boundary where we start ensuring that the current class is enhanced.

Two, I pulled the second boundary check into its own if clause to make it clear that it is detecting when we are done with application code and the verification check as a whole. I think having it in the if/else with the enhancement check did not make it clear that we were not just examining the current class.

In the end, I settled on shouldNotBeEnhanced() as the contested method name, which is pretty close to where we started. I hope that addresses your concern, and I think it works well with the verificationStarted flag.