Skip to content

HDDS-14923. Enable UselessStringValueOf, UseStringBufferForStringAppends in PMD#10007

Draft
ptlrs wants to merge 2 commits intoapache:masterfrom
ptlrs:HDDS-14923-UseStringBufferForStringAppends
Draft

HDDS-14923. Enable UselessStringValueOf, UseStringBufferForStringAppends in PMD#10007
ptlrs wants to merge 2 commits intoapache:masterfrom
ptlrs:HDDS-14923-UseStringBufferForStringAppends

Conversation

@ptlrs
Copy link
Copy Markdown
Contributor

@ptlrs ptlrs commented Mar 30, 2026

What changes were proposed in this pull request?

This PR enables new PMD rules:

  1. UselessStringValueOf No need to call String.valueOf to append to a string where the type can be inferred
  2. UseStringBufferForStringAppends Use StringBuffer for concatenating strings

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14923

How was this patch tested?

CI: https://github.com/ptlrs/ozone/actions/runs/23722077147

@adoroszlai adoroszlai added the code-cleanup Changes that aim to make code better, without changing functionality. label Mar 31, 2026
Copy link
Copy Markdown
Contributor

@Gargi-jais11 Gargi-jais11 left a comment

Choose a reason for hiding this comment

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

Thanks @ptlrs for the patch. Please resolve the conflicts in your branch and find the inline comments.

* @param nextLevel
* @return subpath
*/
@SuppressWarnings("PMD.UseStringBufferForStringAppends")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why to do suppression over here? If we are applying this as a rule we should refactor this as well as the change is not very complex.

public static String buildSubpath(String path, String nextLevel) {
    StringBuilder sb = new StringBuilder();
    if (!path.startsWith(OM_KEY_PREFIX)) {
      sb.append(OM_KEY_PREFIX);
    }
    sb.append(path);
    String normalized = removeTrailingSlashIfNeeded(sb.toString());
    if (nextLevel == null) {
      return normalized;
    }
    return new StringBuilder(normalized)
        .append(OM_KEY_PREFIX)
        .append(nextLevel)
        .toString();
  }

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Apr 1, 2026

Choose a reason for hiding this comment

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

The first StringBuilder is not needed.

String subpath = !path.startsWith(OM_KEY_PREFIX)
    ? OM_KEY_PREFIX + path
    : path;

private static final int PATH_INDENT = 27;

@Override
@SuppressWarnings(value = "PMD.UseStringBufferForStringAppends")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think no need of suppression here as well. Just need to change these below lines 166 - 170 in this method:

String subPath = subPathDU.path("path").asText("");
          // differentiate key from other types
          if (!subPathDU.path("isKey").asBoolean(false)) {
            subPath += OM_KEY_PREFIX;
          }

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ptlrs for the patch.

Comment on lines 69 to +72
if (name == null) {
name = "_" + RDB_CHECKPOINT_DIR_PREFIX + currentTime;
}
checkpointDir += name;
checkpointDir.append(name);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

String concatenation can be changed to use the outer StringBuilder:

      if (name == null) {
        checkpointDir.append('_')
            .append(RDB_CHECKPOINT_DIR_PREFIX)
            .append(currentTime);
      } else {
        checkpointDir.append(name);
      }

boolean topologyAware) {
String key = pipeline.getId().getId().toString() + pipeline.getType();
StringBuilder key = new StringBuilder()
.append(pipeline.getId().getId().toString())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary toString()

Suggested change
.append(pipeline.getId().getId().toString())
.append(pipeline.getId().getId())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-cleanup Changes that aim to make code better, without changing functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants