String vs. StringBuilder vs. Deparser usage

[manticore-projects]

Greetings!

At the moment, I feel like the whole implementation of the DeParsing stuff is not optimal:

1. there are countless String to StringBuilder and StringBuilder to String conversions

2. the toString() methods compete with the DeParser implementations. In the best case they are completely redundant and do the same. In the worst case they produce different output.

Suggestions:

a) maybe we would like to make Statement abstract and introduce an additional abstract method

public StringBuilder appendTo(StringBuilder builder) {
    builder.append(...);
    return builder;
}

public toString() {
    return appendTo(new StringBuilder()).toString();
}

I have actually used that for all recent statements i have added and I am just looking for a formalized design pattern, which we should also mention in the "How to contribute" documentation.

b) Can we somehow remove the Duplication between toString() and the DeParser implementation?

I feel like the easiest way would be to call appendTo(builder) within the DeParser for each entity. This was cheap and simple.

Alternatively, we would call the Deparser inside each appendTo() implementation of the Statements and Expressions. But that feels like 'overdesigned' to me and I can't see the benefit of going that route.

Which way would you like to prefer or should we better not touch that stuff at all?

Again, I would like to agree on a kind of Design Pattern which is also stated in a "How to contribute" document.

[wumpz]

Point one is somehow grown. Nowadays a StringBuilder is not needed anymore since internally string concatenation is transformed to a StringBuilder construct.

Point two: I still stick to #133 (b). But you are right, they should produce the same output.

So problems with both implementations are generating different outputs should be resolved, but I prefer both ways to be alive.

[manticore-projects]

Thank you for your response! I did not know that there was Issue #133 although it shows that the concern is real.

The test suite mostly uses assertCanBeParsedAndDeparsed with a SQL statement provided as String (lets call it sqlStr). So we test sqlStr against toString() and also against the Deparser.

In my opinion, this is redundant because either the test
a) sqlStr against toString() or
b) sqlStr against Deparser would suffice.

I fail to see what we gain while I do experience the code duplication and the additional development effort.

but I prefer both ways to be alive.

So be it.

[manticore-projects]

Concern a) toString() vs. StringBuilder has been solved via PR #1301.