-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: further refactoring for code health improvement #406
Changes from 7 commits
ca5c046
a584e24
c4ebc1f
f4a4375
2a89d63
b300463
721b4c4
a3243d1
0f8a84f
0a94ae4
c88c9c4
86fe5df
664134c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,25 +133,9 @@ private FieldScope doFindGetterField() { | |
String methodName = this.getDeclaredName(); | ||
Set<String> possibleFieldNames = new HashSet<>(3); | ||
if (methodName.startsWith("get")) { | ||
if (methodName.length() > 3 && Character.isUpperCase(methodName.charAt(3))) { | ||
// ensure that the variable starts with a lower-case letter | ||
possibleFieldNames.add(methodName.substring(3, 4).toLowerCase() + methodName.substring(4)); | ||
} | ||
// @since 4.32.0 - conforming with JavaBeans API specification edge case when second character in field name is in uppercase | ||
if (methodName.length() > 4 && Character.isUpperCase(methodName.charAt(4))) { | ||
possibleFieldNames.add(methodName.substring(3)); | ||
} | ||
Comment on lines
-136
to
-143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ No longer an issue: Bumpy Road Ahead |
||
getPossibleFieldNamesStartingWithGet(methodName, possibleFieldNames); | ||
} else if (methodName.startsWith("is")) { | ||
if (methodName.length() > 2 && Character.isUpperCase(methodName.charAt(2))) { | ||
// ensure that the variable starts with a lower-case letter | ||
possibleFieldNames.add(methodName.substring(2, 3).toLowerCase() + methodName.substring(3)); | ||
// since 4.32.0: a method "isBool()" is considered a possible getter for a field "isBool" as well as for "bool" | ||
possibleFieldNames.add(methodName); | ||
} | ||
// @since 4.32.0 - conforming with JavaBeans API specification edge case when second character in field name is in uppercase | ||
if (methodName.length() > 3 && Character.isUpperCase(methodName.charAt(3))) { | ||
possibleFieldNames.add(methodName.substring(2)); | ||
} | ||
Comment on lines
-153
to
-154
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ Getting worse: Code Duplication Why does this problem occur?Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health. Read more. |
||
getPossibleFieldNamesStartingWithIs(methodName, possibleFieldNames); | ||
} | ||
if (possibleFieldNames.isEmpty()) { | ||
// method name does not fall into getter conventions | ||
|
@@ -166,6 +150,30 @@ private FieldScope doFindGetterField() { | |
.orElse(null); | ||
} | ||
|
||
private static void getPossibleFieldNamesStartingWithIs(String methodName, Set<String> possibleFieldNames) { | ||
if (methodName.length() > 2 && Character.isUpperCase(methodName.charAt(2))) { | ||
// ensure that the variable starts with a lower-case letter | ||
possibleFieldNames.add(methodName.substring(2, 3).toLowerCase() + methodName.substring(3)); | ||
// since 4.32.0: a method "isBool()" is considered a possible getter for a field "isBool" as well as for "bool" | ||
possibleFieldNames.add(methodName); | ||
} | ||
// @since 4.32.0 - conforming with JavaBeans API specification edge case when second character in field name is in uppercase | ||
if (methodName.length() > 3 && Character.isUpperCase(methodName.charAt(3))) { | ||
possibleFieldNames.add(methodName.substring(2)); | ||
} | ||
} | ||
|
||
private static void getPossibleFieldNamesStartingWithGet(String methodName, Set<String> possibleFieldNames) { | ||
if (methodName.length() > 3 && Character.isUpperCase(methodName.charAt(3))) { | ||
// ensure that the variable starts with a lower-case letter | ||
possibleFieldNames.add(methodName.substring(3, 4).toLowerCase() + methodName.substring(4)); | ||
} | ||
// @since 4.32.0 - conforming with JavaBeans API specification edge case when second character in field name is in uppercase | ||
if (methodName.length() > 4 && Character.isUpperCase(methodName.charAt(4))) { | ||
possibleFieldNames.add(methodName.substring(3)); | ||
} | ||
} | ||
|
||
/** | ||
* Determine whether the method's name matches the getter naming convention ("getFoo()"/"isFoo()") and a respective field ("foo") exists. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ | |
import java.util.concurrent.atomic.AtomicBoolean; | ||
import java.util.function.Function; | ||
import java.util.function.Predicate; | ||
import java.util.function.Supplier; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Getting better: Overall Code Complexity Why does this problem occur?This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals. Read more. |
||
import java.util.stream.Collectors; | ||
|
||
/** | ||
|
@@ -212,7 +213,6 @@ private ObjectNode buildDefinitionsAndResolveReferences(String designatedDefinit | |
createDefinitionsForAll, inlineAllSchemas); | ||
Map<DefinitionKey, String> baseReferenceKeys = this.getReferenceKeys(mainSchemaKey, shouldProduceDefinition, generationContext); | ||
considerOnlyDirectReferences.set(true); | ||
final boolean createDefinitionForMainSchema = this.config.shouldCreateDefinitionForMainSchema(); | ||
for (Map.Entry<DefinitionKey, String> entry : baseReferenceKeys.entrySet()) { | ||
String definitionName = entry.getValue(); | ||
DefinitionKey definitionKey = entry.getKey(); | ||
|
@@ -227,13 +227,11 @@ private ObjectNode buildDefinitionsAndResolveReferences(String designatedDefinit | |
referenceKey = null; | ||
} else { | ||
// the same sub-schema is referenced in multiple places | ||
if (createDefinitionForMainSchema || !definitionKey.equals(mainSchemaKey)) { | ||
// add it to the definitions (unless it is the main schema that is not explicitly moved there via an Option) | ||
definitionsNode.set(definitionName, generationContext.getDefinition(definitionKey)); | ||
referenceKey = this.config.getKeyword(SchemaKeyword.TAG_REF_MAIN) + '/' + designatedDefinitionPath + '/' + definitionName; | ||
} else { | ||
referenceKey = this.config.getKeyword(SchemaKeyword.TAG_REF_MAIN); | ||
} | ||
Supplier<String> addDefinitionAndReturnReferenceKey = () -> { | ||
definitionsNode.set(definitionName, this.generationContext.getDefinition(definitionKey)); | ||
return this.config.getKeyword(SchemaKeyword.TAG_REF_MAIN) + '/' + designatedDefinitionPath + '/' + definitionName; | ||
}; | ||
referenceKey = getReferenceKey(mainSchemaKey, definitionKey, addDefinitionAndReturnReferenceKey); | ||
Comment on lines
+230
to
+234
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Getting better: Complex Method Why does this problem occur?This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring. Read more.
Comment on lines
+230
to
+234
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Getting better: Bumpy Road Ahead Why does this problem occur?The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health. Read more. |
||
references.forEach(node -> node.put(this.config.getKeyword(SchemaKeyword.TAG_REF), referenceKey)); | ||
} | ||
if (!nullableReferences.isEmpty()) { | ||
|
@@ -260,6 +258,18 @@ private ObjectNode buildDefinitionsAndResolveReferences(String designatedDefinit | |
return definitionsNode; | ||
} | ||
|
||
private String getReferenceKey(DefinitionKey mainSchemaKey, DefinitionKey definitionKey, Supplier<String> addDefinitionAndReturnReferenceKey) { | ||
final String referenceKey; | ||
if (definitionKey.equals(mainSchemaKey) && !this.config.shouldCreateDefinitionForMainSchema()) { | ||
// no need to add the main schema into the definitions, unless explicitly configured to do so | ||
referenceKey = this.config.getKeyword(SchemaKeyword.TAG_REF_MAIN); | ||
} else { | ||
// add it to the definitions | ||
referenceKey = addDefinitionAndReturnReferenceKey.get(); | ||
} | ||
return referenceKey; | ||
} | ||
|
||
/** | ||
* Produce reusable predicate for checking whether a given type should produce an entry in the {@link SchemaKeyword#TAG_DEFINITIONS} or not. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No longer an issue: Complex Method
doFindGetterField is no longer above the threshold for cyclomatic complexity