streamline toString() impl with Deparsers

[yangyangyyy]

right now both implement the same logic. as a general principle, it's bad to repeat the same logic in multiple places.

it seems Deparser does a more complete traversal of the objects, shouldn't we implement all toString() with their corresponding Deparsers ?

[wumpz]

If you are talking only about getting a string representation of the SQL you are right. But there is more. The deparsers are following the visitor pattern and toString does not. At the moment it is a very convinient way to test the visitor hierarchy. That one will not work anymore if you drop the second implementation. Additional recently adapter classes were introduced. The next logical step would be to transfer the visitor logic of deparsers into the adapters. Conclusion: it is a good test pattern to implement the same output using different strategies and test for a match. IMHO this does not violate dry.

[yangyangyyy]

I think as long as you have the same logic defined in two places, you
always run the risk that these surprise u with different results.

your point from the previous comment was that the deparser framework may
be changed from using "visitor" to using "adaptor" , so that if we have a
standalone logic in toString(), the toString() logic does not need to be
changed together with the adaptor. am I understanding it correctly?

but my counter argument to that is, even if u change to adaptor, the new
adaptor-based traversal still runs the risk of producing conflicting
results from toString(). after all, conceptually the core of toString() is
to follow some traversal, and that has been defined elsewhere so it's
better not to do it again(possibLy differently)
On May 17, 2015 2:32 AM, "Tobias" notifications@github.com wrote:

If you are talking only about getting a string representation of the SQL
you are right. But there is more. The deparsers are following the visitor
pattern and toString does not. At the moment it is a very convinient way to
test the visitor hierarchy. That one will not work anymore if you drop the
second implementation. Additional recently adapter classes were introduced.
The next logical step would be to transfer the visitor logic of deparsers
into the adapters. Conclusion: it is a good test pattern to implement the
same output using different strategies and test for a match. IMHO this does
not violate dry.

—
Reply to this email directly or view it on GitHub
#133 (comment)
.

[wumpz]

I understand your argument and agree with you that DRY is a good practice to follow.

At the time I made this fork, both ways were halfway implemented. So I decided to go for both and checking each against the other. My point here is testing, tests, tests, write effective tests without much effort. Without this different strategies and massive tests, the visitors (deparserd) or the toString would not have been in this very good state.
I do not agree that both have the same logic defined. They produce the same output but using different strategies of traversal the object hierarchy. I agree as I said if it would be only the output it would be redundant.

Assume you would remove one way. So how do you test the deparser. You let it build a sql and check it against a fix String or dynamic generated String from a parsed statement. Therefore you need for the dynamic way a different strategy of building this String if you want to avoid checking the deparser against itself. You have to implement some kind of toString method. So why not keeping this dynamic String building within the actual object (toString)?

The fact, that the adapters will be enhanced be the traversal logic of the deparsers was not an argument for keeping the toStrings but the way how they will evolve, so merely an informational statement :).

So at the moment I do not want to change the implementation of it.