Refactoring suggestions

[mattwynne]

This PR contains a few small refactorings reflecting
things I'd like to see us change about the language in the code
as we move from Gherkin 2 to Gherkin 3.

Basically, this just comes down to reducing the amount of
solution-language used in the code, by replacing most of the explicit
references to the visitor pattern. Instead I've used what are, IMO, more
meaningful names for the types and messages.

I've done this as a PR because I didn't feel comfortable pushing it
straight into master without some discussion.

What do you think? Any concerns?

[aslakhellesoy]

Every description and implementation I have seen of the Visitor Pattern uses accept for the method name on the nodes. I don't think renaming it to describeTo makes it any clearer that we're using the Visitor Pattern.

What was your motivation for renaming it?

[aslakhellesoy]

I like the rename from EvalVisitor to Evaluator.

[aslakhellesoy]

Any comments on my comments @mattwynne ?

[mattwynne]

So on the visitor pattern names thing, my view (admittedly influenced by @sf105 and @npryce) is that using the name of the design pattern might actually make it harder to understand the code.

Anyone familiar with the GoF patterns should be able to quickly recognise we're using visitor by reading the code, whatever names we use. Anyone who doesn't might well wonder what that word accept means. What we want the AST to do is to describe itself (include descendants) to the visitor. The message accept just seems more abstract than necessary. That's why I prefer describe_to. I think we could even call it walk.

Similarly, I think the visit_ prefixes on the AST vistor interface (I'm totally happy to call the interface / duck type that) are just noise. As the Evaluator, I know I'm acting as a visitor and I don't need to be reminded of that with every message that comes in. I hadn't considered using overrides of a single method for Java, and I think you're probably right that having a consistent interface is a better idea.

Surely most examples of the visitor pattern use common terminology precisely because they're examples for teaching the pattern, rather than real domain code?

[aslakhellesoy]

Good points regarding naming. Let's do it!

The walking is currently done by the visitor accessing public fields in the nodes, aka external iteration. In Gherkin we may have visitors that prefer a different walking path, so I think it's wise to keep it that way instead of letting the nodes themselves walk children.

Happy with that?

[mattwynne]

Yeah actually I noticed this difference between Bool and the current version of Gherkin, where we only send the attributes of the node to the visitor rather than the node itself. I think as long as the AST nodes are immutable, it's fine to just pass the whole node. Less params in methods has got to be a good thing.

I'll put the JS things back and merge this in.

[ilanpillemer]

Ahh.. I see your comments.

I find pattern mnemonics in class names extremely useful when spelunking code.

And I don't agree that anyone familiar will the GoF patterns when spelunking will immediately see all the patterns - especially when there are many interacting patterns.

Code is complex and hiding the interacting patterns behind purely domain driven domain naming; I don't think simplifies anything; and I don't think it removes noise when reading code. It does remove noise when writing code.

The conventions and mnemonics help the patterns stand out "through" the dominant domain class decomposition.

In terms of the renaming of the accept method I would prefer walk.
describeTo confuses me; and to be frank I think it looks ugly.

How do you feel about changing it to walk.

[ilanpillemer]

I have thought about it some more; and I now agree with you Matt.

I think we should change names to walk and renderer.
I will do that for #32