joelittlejohn / jsonschema2pojo

Generate Java types from JSON or JSON Schema and annotate those types for data-binding with Jackson, Gson, etc
http://www.jsonschema2pojo.org
Apache License 2.0
6.24k stars 1.66k forks source link

Use String concatenation in toString method #994

Closed puce77 closed 3 years ago

puce77 commented 5 years ago

While already in Java 8 (and before) using StringBuilder for single statement concatenation did result in about the same class code as using +, but actually reduced readability, with Java 9 and JEP 280 the generated byte code is potentially even slower.

https://openjdk.java.net/jeps/280

Just use simple concatentation in toString method.

Sample (toString generated by IntelliJ):

` import java.time.Month; import java.util.Objects;

public class TestObject {
    private int intProperty;
    private double doubleProperty;
    private boolean booleanProperty;
    private String nonNullObjectProperty;
    private Double objectProperty;
    private Month enumProperty;

    @Override
    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }
        if (!(o instanceof TestObject)) {
            return false;
        }
        TestObject that = (TestObject) o;
        return intProperty == that.intProperty &&
                Double.compare(that.doubleProperty, doubleProperty) == 0 &&
                booleanProperty == that.booleanProperty &&
                nonNullObjectProperty.equals(that.nonNullObjectProperty) &&
                Objects.equals(objectProperty, that.objectProperty) &&
                enumProperty == that.enumProperty;
    }

    @Override
    public int hashCode() {
        return Objects.hash(intProperty, doubleProperty, booleanProperty, nonNullObjectProperty, objectProperty, enumProperty);
    }

    @Override
    public String toString() {
        return "TestObject{" +
                "intProperty=" + intProperty +
                ", doubleProperty=" + doubleProperty +
                ", booleanProperty=" + booleanProperty +
                ", nonNullObjectProperty='" + nonNullObjectProperty + '\'' +
                ", objectProperty=" + objectProperty +
                ", enumProperty=" + enumProperty +
                '}';
    }
}

`

joelittlejohn commented 3 years ago

I've investigated this one from a few angles and tried to improve the implementation. I've come to the conclusion that we can't really improve this.

In your example, it's true that "TestObject{" + "intProperty=" + intProperty + ", doubleProperty=" ...etc will be rewritten by any modern Java compiler into StringBuilder calls, so there's no reason to use the StringBuilder explicitly in this case, however the toString we generate can actually be much more complicated than this, particularly in the case where there are superclass fields to include and also when any field is not primitive. We end up having conditionals in the toString method, rather than a single expression of many string + operations. In this case, using simple string concatenation would require doing it in stages with +=. The problem with this is that string reassignment with += is in fact slower than using the StringBuilder, so we are back to StringBuilder as the most appropriate option.

The next avenue I explored was replacing some of the many calls to .append with simple string concatenation, so that even if we do use StringBuilder overall, we would append much less often and use arguments like "intProperty=" + intProperty so that a lot of the String concatenation uses +. I agree this would make the implementation more readable. The problem with this approach is that JCodeModel (the library we use to generate Java sources) applies explicit brackets to any binary operation, since this allows JCodeModel to avoid any problems with operator precedence. So the result is a confusing mess of brackets. Here's an example:

        sb.append((((ScalarTypes.class.getName()+'@')+ Integer.toHexString(System.identityHashCode(this)))+'['));
        sb.append(((("stringField"+'=')+((this.stringField == null)?"<null>":this.stringField))+','));
        sb.append(((("numberField"+'=')+((this.numberField == null)?"<null>":this.numberField))+','));
        sb.append(((("integerField"+'=')+((this.integerField == null)?"<null>":this.integerField))+','));
        sb.append(((("booleanField"+'=')+((this.booleanField == null)?"<null>":this.booleanField))+','));
        sb.append(((("nullField"+'=')+((this.nullField == null)?"<null>":this.nullField))+','));

the more + operations you string together, the more brackets you get. So we haven't made much of an improvement and we've introduced some very strange code instead.

I understand that Eclipse and Intellij will generate better results here, but we are somewhat stuck with the limitations of JCodeModel and they aren't. These methods are supposed to be generate-and-forget - they aren't meant for human consumption or modification. In light of the above investigation, I'm inclined to leave this as it is.