HDDS-14923. Enable UselessStringValueOf, UseStringBufferForStringAppends in PMD

[ptlrs]

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

[Gargi-jais11]

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

[Gargi-jais11]

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();
  }

[adoroszlai]

The first StringBuilder is not needed.

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

[Gargi-jais11]

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;
          }

[adoroszlai]

Thanks @ptlrs for the patch.

[adoroszlai]

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);
      }

[adoroszlai]

nit: unnecessary toString()

Suggested change

	.append(pipeline.getId().getId().toString())
	.append(pipeline.getId().getId())

[ptlrs]

Thanks for the reviews @adoroszlai @Gargi-jais11. I have updated the PR. Can you please take another look?

[adoroszlai]

Thanks @ptlrs for updatinig the patch.

[adoroszlai]

• This duplicates path.
• StringBuilder is unnecessary if nextLevel == null, and we can use concatenation in the other case
• Better avoid reassigning parameter path.

So logic can be simplified:

String subpath = !subpath.startsWith(OM_KEY_PREFIX)
    ? OM_KEY_PREFIX + path
    : path;
subpath = removeTrailingSlashIfNeeded(subpath);
return nextLevel != null
    ? subpath + OM_KEY_PREFIX + nextLevel
    : subpath;

[adoroszlai]

I think PMD is really wrong here: previously the (internal) StringBuilder was created only conditionally, but now it is eagerly created. We can avoid that.

String pathValue = subPathDU.path("path").asText("");
boolean isDir = !subPathDU.path("isKey").asBoolean(false);
String subPath = isDir ? (pathValue + OM_KEY_PREFIX) : pathValue;

[Gargi-jais11]

Overall LGTM! Let's update the patch with above comments then its good to go.