How to use stringBuilder

Question

I have a function and Technical Leader review code and said : Why this if statement? It's basically the same message. If you want customized, use a string builder with the type. How to change it , can someone help me?

private Optional<String> validatePrimaryPath(SalesChannelType salesChannelCode, List<String> primaryPathList) {

  if (CollectionUtils.isEmpty(primaryPathList)) {
    if (salesChannelCode.equals(SalesChannelType.HEB_TO_YOU)) {
      return Optional.of("Customer Hierarchy is mandatory field for HebToYou.");
    } else {
      return Optional.of("Customer Hierarchy is mandatory field.");
    }
  }

  return Optional.empty();
}

I would not use a `StringBuilder` for this - or am I missing something? — Scary Wombat, Mar 11 '20 at 03:03
I'm more concerned about the raw types. I also would not use a `StringBuilder` here. — Elliott Frisch, Mar 11 '20 at 03:08
i do not understand why have to use stringBuilder here and how to use it. — khoale, Mar 11 '20 at 03:08
I agree with @Scary Wombat. You could use a single Optional.of and append "for HebToYou" conditionally via ternary operator. Just a thought... — S B, Mar 11 '20 at 03:09

score 2 · Answer 1 · edited Mar 11 '20 at 16:56

Specifying two string literals implies that there is no runtime overhead. You can achieve the same using string concatenation, when all arguments are compile-time constants. In contrast, using a StringBuilder always implies a runtime operation. You can read more about the “StringBuilder is better than + myth” in this answer.

You can generally reduce the syntactic complexity of you code:

private Optional<String> validatePrimaryPath(
    SalesChannelType salesChannelCode, List<String> primaryPathList) {

    final String prefix = "Customer Hierarchy is mandatory field";
    final String general = prefix + ".", forHebToYou = prefix + " for HebToYou.";

    return Optional.of(
            salesChannelCode.equals(SalesChannelType.HEB_TO_YOU)? forHebToYou: general)
        .filter(s -> CollectionUtils.isEmpty(primaryPathList));
}

This emphasizes that you are doing the same, just with slightly different data and uses the semantic of Optional instead of an if statement. And the actual code doesn’t need changes, if you decide the externalize the actual strings.

Note that if SalesChannelType is an enum, there is no need for equals, you can just use salesChannelCode == SalesChannelType.HEB_TO_YOU then.

score 1 · Accepted Answer · answered Mar 11 '20 at 03:12

To prevent writing the same (partial) string literal multiple times, you can:

Use a constant for the common part:

if (CollectionUtils.isEmpty(primaryPathList)) {
    final String COMMON = "Customer Hierarchy is mandatory field";
    if (salesChannelCode.equals(SalesChannelType.HEB_TO_YOU)) {
        return Optional.of(COMMON + " for HebToYou.");
    } else {
        return Optional.of(COMMON + ".");
    }
}

Build the string using StringBuilder:

if (CollectionUtils.isEmpty(primaryPathList)) {
    StringBuilder buf = new StringBuilder("Customer Hierarchy is mandatory field");
    if (salesChannelCode.equals(SalesChannelType.HEB_TO_YOU)) {
        buf.append(" for HebToYou");
    }
    return Optional.of(buf.append('.').toString());
}

Personally, I would keep the code in the question, especially if you ever might need support non-English versions of the text, because in other languages the extra text might not go there.

Even when not using compile-time constants, I wouldn’t use `StringBuilder`, as it complicates the code for no benefit. In all Java versions up to Java 8, you get this code anyway when using `+`, and starting with Java 9, you get even better when using `+` (as discussed [here](https://stackoverflow.com/a/54645780/2711488)). But I like the aspect of not having redundant `Optional.of` calls in the second variant. You can even [simplify the construct to a single statement](https://stackoverflow.com/a/60635152/2711488)… — Holger, Mar 11 '20 at 11:44

score 1 · Answer 3 · answered Mar 11 '20 at 03:12

First, please don't use raw types. Second, I do not agree that using a StringBuilder to build the message is an improvement; however, since that is what you want I will show you what was likely intended. Something like,

private Optional<String> validatePrimaryPath(SalesChannelType salesChannelCode, 
                List<String> primaryPathList) {
    if (CollectionUtils.isEmpty(primaryPathList)) {
        StringBuilder sb = new StringBuilder(
                    "Customer Hierarchy is mandatory field");
        if (salesChannelCode.equals(SalesChannelType.HEB_TO_YOU)) {
            sb.append(" for HebToYou");
        }
        sb.append(".");
        return Optional.of(sb.toString());
    }

    return Optional.empty();
}

Note that I have specified the method returns an Optional<String> and takes a List<String> when you write Optional and List without specifying the type then you are using raw types.

score 0 · Answer 4 · answered Mar 11 '20 at 04:03

Personally, I think your code is OK but I have a suggestion for this if you still want to use StringBuilder

private Optional validatePrimaryPath(SalesChannelType salesChannelCode, List primaryPathList) {
    if (CollectionUtils.isEmpty(primaryPathList)) {
        StringBuilder sb = new StringBuilder("Customer Hierarchy is mandatory field.");
        if (salesChannelCode.equals(SalesChannelType.HEB_TO_YOU)) {
            sb.insert(sb.length()-1, " for HebToYou");
        }
        return Optional.of(sb.toString());
    }
    return Optional.empty();
}

How to use stringBuilder

4 Answers4