Skip to content

Refactoring: fix SonarQube issues in populateMap() #990

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.

Sign up for GitHub

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

Merged
stleary merged 2 commits into from
Jul 1, 2025
+67 −39
Merged

Refactoring: fix SonarQube issues in populateMap() #990

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
916fba5
#984 extract methods reducing cognitive complexity
Simulant87 Jun 25, 2025
5063d31
#984 extract method for annotation value check
Simulant87 Jun 25, 2025
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
106 changes: 67 additions & 39 deletions src/main/java/org/json/JSONObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1780,46 +1780,30 @@ private void populateMap(Object bean, Set<Object> objectsRecord, JSONParserConfi

Method[] methods = includeSuperClass ? klass.getMethods() : klass.getDeclaredMethods();
for (final Method method : methods) {
final int modifiers = method.getModifiers();
if (Modifier.isPublic(modifiers)
&& !Modifier.isStatic(modifiers)
&& method.getParameterTypes().length == 0
&& !method.isBridge()
&& method.getReturnType() != Void.TYPE
&& isValidMethodName(method.getName())) {
final String key = getKeyNameFromMethod(method);
if (key != null && !key.isEmpty()) {
try {
final Object result = method.invoke(bean);
if (result != null || jsonParserConfiguration.isUseNativeNulls()) {
// check cyclic dependency and throw error if needed
// the wrap and populateMap combination method is
// itself DFS recursive
if (objectsRecord.contains(result)) {
throw recursivelyDefinedObjectException(key);
}

objectsRecord.add(result);

testValidity(result);
this.map.put(key, wrap(result, objectsRecord));

objectsRecord.remove(result);

// we don't use the result anywhere outside of wrap
// if it's a resource we should be sure to close it
// after calling toString
if (result instanceof Closeable) {
try {
((Closeable) result).close();
} catch (IOException ignore) {
}
}
final String key = getKeyNameFromMethod(method);
if (key != null && !key.isEmpty()) {
try {
final Object result = method.invoke(bean);
if (result != null || jsonParserConfiguration.isUseNativeNulls()) {
// check cyclic dependency and throw error if needed
// the wrap and populateMap combination method is
// itself DFS recursive
if (objectsRecord.contains(result)) {
throw recursivelyDefinedObjectException(key);
}
} catch (IllegalAccessException ignore) {
} catch (IllegalArgumentException ignore) {
} catch (InvocationTargetException ignore) {

objectsRecord.add(result);

testValidity(result);
this.map.put(key, wrap(result, objectsRecord));

objectsRecord.remove(result);

closeClosable(result);
}
} catch (IllegalAccessException ignore) {
} catch (IllegalArgumentException ignore) {
} catch (InvocationTargetException ignore) {
}
}
}
Expand All @@ -1830,6 +1814,10 @@ private static boolean isValidMethodName(String name) {
}

private static String getKeyNameFromMethod(Method method) {
if (!isValidMethod(method)) {
Copy link
Owner

@stleary stleary Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Method validation now requires two function calls: getKeyNameFromMethod() and isValidMethod(). Also, the validation check is performed twice, in isValidMethod(), and in populateMap() with the key check. The call to isValidMethod() could be moved back up to the original location in populateMap(). This will help restore a bit of performance without losing all of the cognitive load improvements.

Copy link
Contributor Author

@Simulant87 Simulant87 Jul 1, 2025

Choose a reason for hiding this comment

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

I am not 100% sure, how you want to build it.
moving the extracted method isValidMethod() back into the for loop of populateMap() would either indent everything back into an if-statement, which I removed. And reintroducing it would increase the cognitive load again. Or it would introduce an early exit continue; into the for loop, which would be bad for performance as it introduces an unpredictable jump. I tried to avoid both.

return null;
}

final int ignoreDepth = getAnnotationDepth(method, JSONPropertyIgnore.class);
if (ignoreDepth > 0) {
final int forcedNameDepth = getAnnotationDepth(method, JSONPropertyName.class);
Expand All @@ -1840,7 +1828,7 @@ private static String getKeyNameFromMethod(Method method) {
}
}
JSONPropertyName annotation = getAnnotation(method, JSONPropertyName.class);
if (annotation != null && annotation.value() != null && !annotation.value().isEmpty()) {
if (annotationValueNotEmpty(annotation)) {
return annotation.value();
}
String key;
Expand All @@ -1866,6 +1854,46 @@ private static String getKeyNameFromMethod(Method method) {
return key;
}

/**
* checks if the annotation is not null and the {@link JSONPropertyName#value()} is not null and is not empty.
* @param annotation the annotation to check
* @return true if the annotation and the value is not null and not empty, false otherwise.
*/
private static boolean annotationValueNotEmpty(JSONPropertyName annotation) {
return annotation != null && annotation.value() != null && !annotation.value().isEmpty();
}

/**
* Checks if the method is valid for the {@link #populateMap(Object, Set, JSONParserConfiguration)} use case
* @param method the Method to check
* @return true, if valid, false otherwise.
*/
private static boolean isValidMethod(Method method) {
final int modifiers = method.getModifiers();
return Modifier.isPublic(modifiers)
&& !Modifier.isStatic(modifiers)
&& method.getParameterTypes().length == 0
&& !method.isBridge()
&& method.getReturnType() != Void.TYPE
&& isValidMethodName(method.getName());
}

/**
* calls {@link Closeable#close()} on the input, if it is an instance of Closable.
* @param input the input to close, if possible.
*/
private static void closeClosable(Object input) {
// we don't use the result anywhere outside of wrap
// if it's a resource we should be sure to close it
// after calling toString
if (input instanceof Closeable) {
try {
((Closeable) input).close();
} catch (IOException ignore) {
}
}
}

/**
* Searches the class hierarchy to see if the method or it's super
* implementations and interfaces has the annotation.
Expand Down
Loading