Store annotationType.getSimpleName() in a field in BooleanExecutionCondition

[Adagedo]

Overview

My PR optimizes the performance and readability of the BooleanExecutionCondition class by memoizing the annotationType.SimpleName method.
I did not alter any logic though. I only introduced a lightweight internal optimization .

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[sbrannen]

Congratulations on submitting your first PR for JUnit! 👍

My PR optimizes the performance and readability of the BooleanExecutionCondition class by memoizing the annotationType.SimpleName method.

There is no performance gain with that change since annotationType.getSimpleName() is only invoked once.

I would also argue that it does not really improve the readability of the code.

However, the two subclasses of BooleanExecutionCondition both declare private final String annotationName fields.

Thus, if you change the annotationName field you have introduced to protected and modify AbstractJreCondition and AbstractJreRangeCondition so that they no longer declare or set the annotationName field, I would reconsider accepting this PR.

[sbrannen]

Please do not modify lines of code unrelated to your change, and please do not add or remove blank lines.

[Adagedo]

alright thank you, my apologies, it was unoticed

[sbrannen]

Please restore this blank line.

[Adagedo]

Ok I will do just that

[vlsi]

@sbrannen , it would be great to add that into .editorconfig so IDEs trim the whitespace automatically.

I've filed a PR for that: #4623

[sormuras]

Micro "optimization" shifting the work from the method (maybe not called) to the constructor (always called). Let's not do it - keep it as-is.

[sbrannen]

Micro "optimization" shifting the work from the method (maybe not called) to the constructor (always called). Let's not do it - keep it as-is.

@sormuras, I think what I proposed in #4619 (review) is worth doing.

[sormuras]

I'd rather convert those fields into variables. Why keep/introduce an extra state for something that's only used locally in a single method?

[sbrannen]

Why keep/introduce an extra state for something that's only used locally in a single method?

If we create a private field in in BooleanExecutionCondition like in the original proposal in this PR, then I'd agree with you.

However, AbstractJreCondition and AbstractJreRangeCondition currently both store the simple name in private fields.

So, if we want to continue to store the simple name in fields, we should just store the simple name once in a protected field in the common superclass.

I'd rather convert those fields into variables.

If we don't want to continue storing the simple name in fields in any of the affected classes in that hierarchy, yes, we could calculate the simple name based on super.annotationType.getSimpleName() in AbstractJreCondition.validatedVersions() and AbstractJreRangeCondition.isCurrentVersionWithinRange() and leave BooleanExecutionCondition.enabledByDefault() as is.

I'm also OK with going with that approach.

@marcphilipp and @sormuras, WDYT?

[marcphilipp]

I'd rather convert those fields into variables.

If we don't want to continue storing the simple name in fields in any of the affected classes in that hierarchy, yes, we could calculate the simple name based on super.annotationType.getSimpleName() in AbstractJreCondition.validatedVersions() and AbstractJreRangeCondition.isCurrentVersionWithinRange() and leave BooleanExecutionCondition.enabledByDefault() as is.

I'm also OK with going with that approach.

I'd prefer that approach. 👍

[marcphilipp]

Team decision: @sbrannen himself will change the code according to his proposal in #4619 (comment).

[sbrannen]

@Adagedo, thanks again for the PR, even though we decided to address this differently.

Team decision: @sbrannen himself will change the code according to his proposal in #4619 (comment).

Done in 483fcc9.

[marcphilipp]

@Adagedo, thanks again for the PR, even though we decided to address this differently.

+1 👍

[Adagedo]

@sbrannen My pleasure