Skip to content
This repository has been archived by the owner on May 28, 2019. It is now read-only.

refactoring of names for visitor stuff to domain oriented names #34

Closed
wants to merge 2 commits into from

Conversation

ilanpillemer
Copy link
Contributor

I have refactored the visitor names as per discussion that happened in #32
Essentially I have tried to replace the standard visitor nomenclature used for showing how visitors work with domain related naming that shows how the visitor pattern is being applied in the code.

@os97673
Copy link
Contributor

os97673 commented Feb 22, 2013

User method overriding is perfectly fine in Java, but can not be used in Ruby or JavaScript. This means we will have different names for the same method in different languages :( Is this what we want?

@ilanpillemer
Copy link
Contributor Author

We have different names anyway. describe_to is not describeTo; Not is not not_node. We have different class structures and different ways of writing functions as well.

@ilanpillemer
Copy link
Contributor Author

Also I think walk in java which is overridden for each Expr; is clearly mapped to walk_var, walk_or etc. If thats not clear to someone reading the code I would be very surprised.

@ilanpillemer
Copy link
Contributor Author

And lastly, I don't think we should cripple the Java code base, because dynamic language can't override functions. We should use the features of the language we work with. Java is a statically typed language; and so should be used as such and the languages features should be used correctly. IMO.

@os97673
Copy link
Contributor

os97673 commented Feb 22, 2013

I agree that mapping "walk" to "walk_var"/"walk_or" is rather intuitive. Are you going to change names for other languages?

@ilanpillemer
Copy link
Contributor Author

I did. Its in the commit.

@os97673
Copy link
Contributor

os97673 commented Feb 22, 2013

Oops, (to long don't read :(
I need to learn to show diff stats so it would be clear how many files have been changed.

@mattwynne
Copy link
Contributor

I don't really understand the value of this rename. If the AST were actually walking the visitor around the tree (the way it does in the current version of Cucumber) I think that this new method name would help make the code more understandable, but all we're currently asking the nodes to do is use double-dispatch to describe themselves to the caller.

I disagree that describe_to and describeTo are not the same: they are the idiomatic way to name things in each of the respected languages.

I'm ambivalent about whether or not to use overrides in Java, but I think @aslakhellesoy already made the point in #12 that it helps if we try to make the shape of the code consistent between the different languages as much as possible.

@ilanpillemer
Copy link
Contributor Author

Visitor calls visit; and Walker calls walk; and the node uses the walkWith request. We are asking the Walker to walk the node.

I am not ambivalent about using overrides in Java. I do not think that effects the shape.

@ilanpillemer
Copy link
Contributor Author

@mattwynne hmmm. Let me try explain this another way.

Lets say we were using the Visitor pattern twice; for our abstract tree.
If we had Walker, walk and walkWith for the standard one; we could then add a new visitor in called EggScrambler, the implementations would have a eggScramble method and the double dispatch would be invoked by a eggScrambleWith call, taking a EggScrambler in as a parameter.

If we use the current approach of just calling the visit methods after the Node - things could start getting very confusing.

Essentially the Visitor is visiting; and getting invoked via double dispatch with accept. It seems to me this logically maps to to: a Walker that is walking and getting invoked by double dispatch with a walkWith method.

In summary the Visitor is doing the visiting. The node is passive.. it is not describing itself, it is handing over control via the accept method to the Visitor which acts. describeTo does not seem to capture that subtlety to me.

Thoughts?

@aslakhellesoy
Copy link
Contributor

I appreciate everyone's desire and effort to come up with good names. In this case, describe_to, walk, walk_with, Walker only seem to generate confusion and discussion.

Let's just revert to the GoF terminology. It's well-documented and it's what most people are used to. Having thought about it I don't think domain-specificness is particularly useful or important here. It's more important that we use terminology people can recognise in pattern literature.

Pseudo code:

class SomethingVisitor implements Visitor
  def visit_foo(foo, arg)
  end
  def visit_bar(bar, arg)
  end
end

class Foo
  def accept(visitor, arg)
    return visitor.visit_foo(this, arg)
  end
end

Is that so terrible?

@ilanpillemer
Copy link
Contributor Author

No its not terrible at all.

What about Java method overloading. I would prefer having just a visit method for Java rather than all these different names to emulate non statically typed languages.

It does not seem to effect the code shape to me?
What am I missing in my understanding?

@paoloambrosio
Copy link
Contributor

Since we cannot agree on a different naming, I believe that we should use the classic Visitor as described in the GoF book.

As long as we are using the same class name, I think that we should use overloading when possible. Ruby developers would read a familiar class not using overloading, Java developers would feel comfortable with the same method name.

@mattwynne
Copy link
Contributor

Well, I think it's a shame, because my comments in #32 still stand. Using the pattern name, especially in the method and type names, just feels like hungarian notation to me.

But I agree, there's no point in bickering about it. Let's wait until we have some real code in Gherkin: it's too easy to get into bike-shedding with this toy example.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants