Skip to content
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

Merged
merged 13 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@
<!--<module name="FinalParameters" />-->
<module name="InnerAssignment" />
<module name="CyclomaticComplexity">
<property name="max" value="16"/>
<property name="max" value="9"/>
<property name="switchBlockAsSingleDecisionPoint" value="true"/>
</module>
</module>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

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

Comment on lines -136 to -143

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No longer an issue: Bumpy Road Ahead
doFindGetterField is no longer above the threshold for logical blocks with deeply nested code

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ Getting worse: Code Duplication
introduced similar code in: getPossibleFieldNamesStartingWithGet,getPossibleFieldNamesStartingWithIs

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
Expand All @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Getting better: Overall Code Complexity
The mean cyclomatic complexity decreases from 4.40 to 4.09, threshold = 4

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;

/**
Expand Down Expand Up @@ -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();
Expand All @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Getting better: Complex Method
buildDefinitionsAndResolveReferences decreases in cyclomatic complexity from 11 to 9, threshold = 9

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Getting better: Bumpy Road Ahead
buildDefinitionsAndResolveReferences decreases from 3 to 2 logical blocks with deeply nested code, threshold is one single block per function

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()) {
Expand All @@ -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.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ArrayNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.github.victools.jsonschema.generator.impl.Util;
import com.github.victools.jsonschema.generator.naming.SchemaDefinitionNamingStrategy;
import java.lang.annotation.Annotation;
import java.math.BigDecimal;
Expand Down Expand Up @@ -361,7 +362,7 @@ CustomDefinition getCustomDefinition(ResolvedType javaType, SchemaGenerationCont
@Deprecated
default ResolvedType resolveTargetTypeOverride(FieldScope field) {
List<ResolvedType> result = this.resolveTargetTypeOverrides(field);
return result == null || result.isEmpty() ? null : result.get(0);
return Util.isNullOrEmpty(result) ? null : result.get(0);
}

/**
Expand All @@ -374,7 +375,7 @@ default ResolvedType resolveTargetTypeOverride(FieldScope field) {
@Deprecated
default ResolvedType resolveTargetTypeOverride(MethodScope method) {
List<ResolvedType> result = this.resolveTargetTypeOverrides(method);
return result == null || result.isEmpty() ? null : result.get(0);
return Util.isNullOrEmpty(result) ? null : result.get(0);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.WeakHashMap;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand All @@ -44,6 +45,7 @@ public class TypeContext {

private final TypeResolver typeResolver;
private final MemberResolver memberResolver;
private final WeakHashMap<ResolvedType, ResolvedTypeWithMembers> typesWithMembersCache;
private final AnnotationConfiguration annotationConfig;
private final boolean derivingFieldsFromArgumentFreeMethods;

Expand Down Expand Up @@ -87,6 +89,7 @@ private TypeContext(AnnotationConfiguration annotationConfig, boolean derivingFi
this.memberResolver = new MemberResolver(this.typeResolver);
this.annotationConfig = annotationConfig;
this.derivingFieldsFromArgumentFreeMethods = derivingFieldsFromArgumentFreeMethods;
this.typesWithMembersCache = new WeakHashMap<>();
}

/**
Expand Down Expand Up @@ -129,6 +132,16 @@ public final ResolvedType resolveSubtype(ResolvedType supertype, Class<?> subtyp
* @return collection of (resolved) fields and methods
*/
public final ResolvedTypeWithMembers resolveWithMembers(ResolvedType resolvedType) {
return this.typesWithMembersCache.computeIfAbsent(resolvedType, this::resolveWithMembersForCache);
}

/**
* Collect a given type's declared fields and methods for the inclusion in the internal cache.
*
* @param resolvedType type for which to collect declared fields and methods
* @return collection of (resolved) fields and methods
*/
private ResolvedTypeWithMembers resolveWithMembersForCache(ResolvedType resolvedType) {
return this.memberResolver.resolve(resolvedType, this.annotationConfig, null);
}

Expand Down
Loading
Loading