Comment From: wind57

This looks like a small PR, but there are many things going on.

  • First of all I got rid of the String literals like EMPTY_STRING and friends. These were used because they were replicated in each nullSafeToString method. Since the code has changed, there is no real need for them.

  • Then there is the fact that each of the nullSafeToString documentation was almost identical to Arrays::toString, with the only change that {} is used instead of [].

  • If I take the above argument and simply delegate all of the calls to a common : innerNullSafeToString (via first calling Arrays::toString) and write a JMH test:

import java.util.Arrays;
import java.util.StringJoiner;
import java.util.concurrent.TimeUnit;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;

@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Warmup(iterations = 10, time = 10)
@Measurement(iterations = 10, time = 10)
@State(Scope.Benchmark)
@BenchmarkMode(Mode.AverageTime)
public class NullSafeToString {

    public static void main(String[] args) throws Exception {
        Options opt = new OptionsBuilder()
            .include(NullSafeToString.class.getSimpleName())
            .build();

        new Runner(opt).run();

    }

    @Param(value = {"-1", "0", "10", "100", "1000"})
    int size;

    @Fork(3)
    @Benchmark
    public String nullSafeToString() {

        byte[] array;
        if (size == -1) {
            array = null;
        } else {
            array = new byte[size];
            Arrays.fill(array, (byte) 1);
        }

        if (array == null) {
            return "null";
        }
        int length = array.length;
        if (length == 0) {
            return "{}";
        }
        StringJoiner stringJoiner = new StringJoiner(", ", "{", "}");
        for (short s : array) {
            stringJoiner.add(String.valueOf(s));
        }
        return stringJoiner.toString();
    }

    @Fork(3)
    @Benchmark
    public String refactored() {

        byte[] array;
        if (size == -1) {
            array = null;
        } else {
            array = new byte[size];
            Arrays.fill(array, (byte) 1);
        }

        return innerNullSafeToString(Arrays.toString(array));

    }

    private String innerNullSafeToString(String inter){
        if ("null".equals(inter)) {
            return inter;
        }
        if ("[]".equals(inter)) {
            return "{}";
        }
        return "{" + inter.substring(1, inter.length() - 1) + "}";
    }

}

It proves that this is by far, faster:

Benchmark                          (size)  Mode  Cnt      Score     Error  Units
NullSafeToString.nullSafeToString      -1  avgt   30      4.014 ±   0.057  ns/op
NullSafeToString.nullSafeToString       0  avgt   30      7.361 ±   0.097  ns/op
NullSafeToString.nullSafeToString      10  avgt   30    312.527 ±   4.560  ns/op
NullSafeToString.nullSafeToString     100  avgt   30   2643.421 ±  27.422  ns/op
NullSafeToString.nullSafeToString    1000  avgt   30  26362.922 ± 259.091  ns/op
NullSafeToString.refactored            -1  avgt   30      3.974 ±   0.053  ns/op
NullSafeToString.refactored             0  avgt   30      7.140 ±   0.151  ns/op
NullSafeToString.refactored            10  avgt   30    145.022 ±   3.540  ns/op
NullSafeToString.refactored           100  avgt   30   1026.884 ±  10.899  ns/op
NullSafeToString.refactored          1000  avgt   30  10714.467 ± 120.413  ns/op

  • instead of doing the if/else checks between the types, register the types before hand, into a Map. The value of the Map is a Function that invokes the proper method (the cast is of course needed in the form : (Function<boolean[], String>) ObjectUtils::nullSafeToString). The slight problem with this approach is that the value will have a wildcard and the only way to get around that is via a "wild card capture" in innerWildcardCapture method.

  • Then comes the fact that we can't get rid of the : 

if (obj instanceof Object[]) {
     return nullSafeToString((Object[]) obj);
}

check. Arrays are covariant so this has to stay.

  • The concern I have is here:
    public static String nullSafeToString(@Nullable char[] array) {
        return innerNullSafeToString(Arrays.toString(array));
    }

and the test associated to it:

    @Test
    void nullSafeToStringWithCharArray() {
        char[] array = {'A', 'B'};
        assertThat(ObjectUtils.nullSafeToString(array)).isEqualTo("{A, B}");
    }

The test used to be : assertThat(ObjectUtils.nullSafeToString(array)).isEqualTo("{'A', 'B'}");

Notice tha 'A' and 'B'. The documentation of the method did not specify a clear contract of what the result should be; as a matter of fact since the documentation resembles so much Arrays::toString documentation; this looks like a fairly safe change to me (though somehow breaking for potential callers). If this is indeed a concern, I can revert just this one.

Comment From: snicoll

Thanks for the PR but we prefer to keep things as they are. In particular, we don't like the map/optional lookup in replacement of straightforward if/else.

Comment From: wind57

no worries, I can't believe that someone came to review these 3 years after! awesome