Improving a tutorial on the Visitor pattern

[Paul]

A website promoting a book on design patterns has the code below. Presumably the virtual destructor has been incorrectly omitted, but that might be forgiven because it's not really relevant to the main point.

However, I am concerned about the code duplication here -- the line v.visit(this) is just repeated verbatim regardless of what member of the Element hierarchy v represents. This doesn't strike me as good design.

Any ideas of how to resolve this? (Feel free to suggest ideas that break the Visitor pattern).

Thank You,

Paul

#include <iostream>
#include <string>
using namespace std;

// 1. Add an accept(Visitor) method to the "element" hierarchy
class Element
{
public:
virtual void accept(class Visitor &v) = 0;
};

class This: public Element
{
public:
/*virtual*/void accept(Visitor &v);
string thiss()
{
return "This";
}
};

class That: public Element
{
public:
/*virtual*/void accept(Visitor &v);
string that()
{
return "That";
}
};

class TheOther: public Element
{
public:
/*virtual*/void accept(Visitor &v);
string theOther()
{
return "TheOther";
}
};

// 2. Create a "visitor" base class w/ a visit() method for every "element" type
class Visitor
{
public:
virtual void visit(This *e) = 0;
virtual void visit(That *e) = 0;
virtual void visit(TheOther *e) = 0;
};

/*virtual*/void This::accept(Visitor &v)
{
v.visit(this);
}

/*virtual*/void That::accept(Visitor &v)
{
v.visit(this);
}

/*virtual*/void TheOther::accept(Visitor &v)
{
v.visit(this);
}

// 3. Create a "visitor" derived class for each "operation" to do on "elements"
class UpVisitor: public Visitor
{
/*virtual*/void visit(This *e)
{
cout << "do Up on " + e->thiss() << '\n';
}
/*virtual*/void visit(That *e)
{
cout << "do Up on " + e->that() << '\n';
}
/*virtual*/void visit(TheOther *e)
{
cout << "do Up on " + e->theOther() << '\n';
}
};

class DownVisitor: public Visitor
{
/*virtual*/void visit(This *e)
{
cout << "do Down on " + e->thiss() << '\n';
}
/*virtual*/void visit(That *e)
{
cout << "do Down on " + e->that() << '\n';
}
/*virtual*/void visit(TheOther *e)
{
cout << "do Down on " + e->theOther() << '\n';
}
};

int main()
{
Element *list[] =
{
new This(), new That(), new TheOther()
};
UpVisitor up; // 4. Client creates
DownVisitor down; // "visitor" objects
for (int i = 0; i < 3; i++)
// and passes each
list[i]->accept(up);
// to accept() calls
for (int i = 0; i < 3; i++)
list[i]->accept(down);
}

[Paul]

On Friday, November 7, 2014 11:14:04 PM UTC, Paul wrote:
> A website promoting a book on design patterns has the code below. Presumably the virtual destructor has been incorrectly omitted, but that might be forgiven because it's not really relevant to the main point.
>
> However, I am concerned about the code duplication here -- the line v.visit(this) is just repeated verbatim regardless of what member of the Element hierarchy v represents. This doesn't strike me as good design.
>
> Any ideas of how to resolve this? (Feel free to suggest ideas that break the Visitor pattern).
>
> Thank You,
>
> Paul

Sorry, v does not represent a member of the Element hierarchy, of course. I should have said that the code is duplicated regardless of which member of the Element hierarchy is calling accept -- my question still stands, though.

[Öö Tiib]

On Friday, November 7, 2014 11:14:04 PM UTC, Paul wrote:
> A website promoting a book on design patterns has the code
> below. Presumably the virtual destructor has been incorrectly
> omitted, but that might be forgiven because it's not really
> relevant to the main point.

Looking at 'main' the code has chosen to leak memory instead
of messing with destructors anyway. I would avoid reading
that site.

> However, I am concerned about the code duplication here --
> the line v.visit(this) is just repeated verbatim regardless
> of what member of the Element hierarchy v represents.

Such double dispatch is needed for to ensure that correct
overload from visitor is selected by compiler. There are
no "multimethods" in C++ and so such duplicated one-liner
functions are necessary evil for to achieve it.

> This doesn't strike me as good design.

Visitor pattern can be quite useful when the things
what visitors visit do not belong to same inheritance
hierarhy and they form far more complex object graph
in running program than raw array of 3.

> Any ideas of how to resolve this? (Feel free to suggest
> ideas that break the Visitor pattern).

The first alternative is to have all classes visited
in same inheritance hierarchy (in example they already are,
but in real application that may be quite expensive goal)
and then have virtual member functions like 'up' and 'down'
in all of them.

The second alternative means manually made polymorphism
from outside and that involves switch-cases on 'typeid'
(or self-made "kind") or if-else-if chains on
'dynamic_cast' and that will be very terrible code in
long run. If you see such thing somewhere then better
refactor it into virtual functions or visitor pattern.

Given example is too abstract, useless and meaningless
so it all is just pointless bloat, not worth resolving:

#include <iostream>
int main()
{
std::cout << "do Up on This\n"
"do Up on That\n"
"do Up on TheOther\n"
"do Down on This\n"
"do Down on That\n"
"do Down on TheOther\n";
}

Same effect, 10 times shorter and no memory leaks.

[Paul]

Thanks a lot for your reply. I've never seen a visitor example where the things that visitors visit are not part of the same hierarchy but I see that the basic pattern that when you accept a visitor, you ask the visitor to visit, does not depend on the visited things being in the same hierarchy.

The code on the website is even worse than you think. The original version (before I corrected it in the posting) has a for(i = 0; ...) loop instead of for(int i = 0; ...) and therefore doesn't compile.

Another visitor illustration that is prominent on google searches is http://alumni.cs.ucr.edu/~lgao/teaching/visitor.html

This does not leak memory and does use a virtual destructor. I would hope that this second example is ok but I haven't had a chance to run it yet. I'd be interested to hear opinions on this second example.

If the second example handles the topic well, perhaps a good tip is that it's a good idea to look at univ or college websites because the author is likely to have received extensive feedback from students. I would guess that students are far more likely to correct errors than readers of books.

I'm about to read the Gang-of-four book on design patterns to learn about design patterns in c++. If anyone wants to tell me: "No, read ... instead" then I'm all ears.

Paul

[jacob navia]

Le 08/11/2014 00:13, Paul a écrit :


Sorry to answer a question with another question but isn't the visitor
pattern best implemented in the form of an iterator?

Isn't an iterator "THE" visitor pattern?

Thanks

[Paul]

I'm not sure that I understand you. The original code contains iteration through a list as below (at the end of this message). Whenever I've seen the Visitor pattern, I've seen a class hierarchy of visitors and a class hierarchy of visited-things.

However, the first reply to my post said : "The Visitor pattern can be quite useful when the things that visitors visit do not belong to the same inheritance hierarchy." If that is the statement that concerns you, the question is one for Oo Tiib.

Paul

[Paul]

Sorry, I used careless terminology yet again. I should have said that the original code iterates through an array (not a list). However, I still don't get Jacob Navia's point but I'm very grateful to him for contributing to the discussion.

Paul

[jacob navia]

Well, my point is that the iterator *is* the visitor pattern, i.e. as
you say, "the original code *iterates* through an array".

In the iterator, the line you are trying to avoid is *implicit*, it is
the iterator that applies the visitor to each element...

[Öö Tiib]

Yes. Sometimes some artificial base class (like "Object", "Element",
"Item" or "Visitable") is used for all the visited types but there are
actually no need for that.

> The code on the website is even worse than you think. The original
> version (before I corrected it in the posting) has a for(i = 0; ...)
> loop instead of for(int i = 0; ...) and therefore doesn't compile.
>
> Another visitor illustration that is prominent on google searches
> is http://alumni.cs.ucr.edu/~lgao/teaching/visitor.html
>
> This does not leak memory and does use a virtual destructor. I would
> hope that this second example is ok but I haven't had a chance to run
> it yet. I'd be interested to hear opinions on this second example.
>
> If the second example handles the topic well, perhaps a good tip is that
> it's a good idea to look at univ or college websites because the author
> is likely to have received extensive feedback from students. I would
> guess that students are far more likely to correct errors than readers
> of books.
>
> I'm about to read the Gang-of-four book on design patterns to learn
> about design patterns in c++. If anyone wants to tell me: "No,
> read ... instead" then I'm all ears.

All the patterns are just for solving some particular programming problems.
Imagine case that your more or less mature application contains some large
data hierarchy.

For example there are "Houses", house consists of "Rooms", those have
"Furniture" and "Equipment" in them, somewhere there is "Closet", it may
have "Drawers", some of those contain "Socks" or there is "Fridge" that
contains "Shelves" that contain "Foods" and "Beverages". All of it is more
or less dynamic run-time.
Now you need to locate and pick up all empty beer bottles in particular
house at particular moment. For that you may have to "visit" all places
where empty beer bottle can be in the house. You may write "EmptyBeerBottleFindingVisitor" or some more generic "SearchVisitor".

Most important is that you had no idea that you will need it when you
started to write the program. There are numerous other ways how to
implement it (and lot of those are not bad). You can extend your program with
special tracking/book-keeping for all instances of objects. You may make
all "containers" and "locations" recursively "searchable".

What you should pick? No one can tell for sure. It depends on what is
simpler and more fruitful with your almost mature code base.

[Öö Tiib]

I already trashed the example; with short 3 object array of objects
in same inheritance tree and pure abstract base visitor it is so
hopelessly far from both hint of various responsibilities that
visitors are capable of taking and difficulties that visitors may
need to solve and ways to solve those. It was so stupid that one
of most complex patterns could be easily confused with one of
most simple things.

Certainly there is difference. Iterator is over particular container.
It is very blunt and simple. Most iterators are just wrappers around
single pointer. The code is short since an iterator needs only to
know how to navigate in that container.

The visitors typically visit (part of) runtime object graph. A visitor
visiting object of type X that may point at objects of type Y
can go and visit those too. All that visitable classes have to contain
is one-liner "accept" member function; rest of the responsibilities
are upon the visitor. Once the classes are made visitable by base
visitor there are no changes needed in the code of classes for to
add yet another visitor that can visit with other goal. However the
visitors need to be familiar with all the classes visited.

The navigation may be far more trickier than business of iterator
since data of real applications is not always a simple tree. Several
of X may have pointer to same Y or Y may point to Z that may point
back to one of X. On such cases the visitors need to be smart and
to keep track of places been at to reduce the work and to avoid
hanging lost forever in that runtime data maze with such loops.
Therefore it is wise to keep that smartness of navigation in base
visitor. When data structure is changed then most related changes
will be in base visitor and actual visitors need to be changed only
if new types to visit are added.

One may say that they never have such loops and it is program
design error. So? We are not gods, we make typos. How to find
out that our program does not have such things run-time? Easiest
answer is "with visitor". ;-)

[Tobias Müller]

Paul <peps...@gmail.com> wrote:
> However, I am concerned about the code duplication here -- the line
> v.visit(this) is just repeated verbatim regardless of what member of the
> Element hierarchy v represents. This doesn't strike me as good design.

The point of the visitor pattern is that the accept method is _not_ the
same on every class in the hierarchy. It just _looks_ the same because the
visit method is overloaded.
In a language without overloading this would be obvious, there would have
to be a visitThis(), visitThat(), visitOther() method.

Tobi