2
\$\begingroup\$

I have referred some class diagram to actually create visitor.

#include <iostream>

using namespace std; //know should not be used, but for simplicity

class Book;
class Pen;
class Computer;

class ICartVisitor
{
public:
    virtual int Visit(Book &) = 0;
    virtual int Visit(Pen &) = 0;
    virtual int Visit(Computer &) = 0;
};

class IElement
{
public:

    virtual int accept (ICartVisitor& cartVisitor) = 0;
    virtual int getPrice () = 0;

};

class Book : public IElement
{

public:

    int accept (ICartVisitor& cartVisitor)
    {
        return cartVisitor.Visit(*this);
    }

    int getPrice ()
    {
        return 100;
    }
};

class Pen : public IElement
{
public:

    int accept (ICartVisitor& cartVisitor)
    {
        return cartVisitor.Visit(*this);

    }

    int getPrice ()
    {
        return 5;
    }
};

class Computer : public IElement
{
public:

    int accept (ICartVisitor& cartVisitor)
    {
        return cartVisitor.Visit(*this);

    }

    int getPrice ()
    {
        return 10000;
    }
};

class CartVisitor : public ICartVisitor
{
public:

    //inlining all the functions for simplicity
    int Visit(Book & book) 
    {
        return book.getPrice();
    }

    int Visit(Pen & pen) 
    {
        return pen.getPrice();

    }

    int Visit(Computer & computer) 
    {
        return computer.getPrice();

    }
};


int main(int argc, char* argv[])
{
    int total = 0;

    ICartVisitor *cartVisitor = new CartVisitor();

    total += cartVisitor->Visit(*(new Computer()));
    total += cartVisitor->Visit(*(new Book()));
    total += cartVisitor->Visit(*(new Pen()));

    cout<<total;

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Why are you using visitor here? Do you actually have an idea what it is meant for? \$\endgroup\$ – Zulan Nov 19 '15 at 8:45
  • \$\begingroup\$ @Zulan I am just trying to get hands dirty with Design Patterns. \$\endgroup\$ – Pranit Kothari Nov 19 '15 at 8:48
3
\$\begingroup\$

Stop using new

    // No No No
    ICartVisitor *cartVisitor = new CartVisitor();

Just create a local object:

    ICartVisitor  cartVisitor; 

Again. You don't need to dynamically create objects.

    total += cartVisitor->Visit(*(new Computer()));

    // In this case a temporary object will work.
    total += cartVisitor->Visit(Computer());  // Creates a temporary object
                                              // That is correctly destroyed.

Visitor pattern

The visitor pattern is good when your class hierarchy is stable and does not change much (or at all).

Also the interface that indicates you accept a visitor usually only has the accept() method. Seems like you are mixing two interfaces into IElement.

class IElement
{
public:
    virtual int accept (ICartVisitor& cartVisitor) = 0;
    virtual int getPrice () = 0;

};

I don't see anything wrong with your visitor pattern. But I would not expect it to return an int (it can just a bit unexpected). This is because usually when you visit an object it means that object will have sub-objects that need to be visited. Rather than returning state via the calls to visit() you mutate the state of the visitor object. When you are finished visting all the object just query the visitor about its state.

class ICartVisitor
{
public:
    virtual void Visit(ShoopingCart& cart) = 0;
    virtual void Visit(Book &)             = 0;
    virtual void Visit(Pen &)              = 0;
    virtual void Visit(Computer &)         = 0;
};

Then an implementation would look like this:

class CartVisitor : public ICartVisitor
{
    int costOfItemsInCart;
public:
    CartVisitor()
       : costOfItemsInCart(0)
    {}

    int getCostOfCart() const {return costOfItemsInCart;}

    void Visit(ShoopingCart & cart) 
    {
        // Nothing happens you can't buy a cart.
        // Note it is the accept on the shopping cart that
        // calls the visit on all the items in the cart.
        // The visitor does not need to know anything about
        // the carts implementation. It just knows that each
        // object knows about its own sub objects and will be
        // passed to them via the accept method.
    }

    void Visit(Book & book) 
    {
        //  Book does not need a virtual `getPrince()` method
        //  Being the visitor of Book you know that it is a book
        //  and its whole interface. You don't need to implement
        //  a limited interface (you have the whole Book interface).

        //  You just need to have a book with `getPrice()` method on it.
        //  for this particular implementation of the Visitor.
        //  Other visitors may not need the price (they may be calculating
        //  the weight of the cart for determining shipping costs).
        costOfItemsInCart += book.getPrice();
    }

    void Visit(Pen & pen) 
    {
        costOfItemsInCart += pen.getPrice();
    }

    void Visit(Computer & computer) 
    {
        costOfItemsInCart += computer.getPrice();
    }
};
\$\endgroup\$
  • 1
    \$\begingroup\$ Considering you agree to the example for a visitor pattern, could you please elaborate what is the point of a visitor pattern without any object hierarchy / composition? \$\endgroup\$ – Zulan Nov 20 '15 at 7:50
  • \$\begingroup\$ @Zulan: I am not sure what you mean. Are you referring to when your class hierarchy is stable and does not change much \$\endgroup\$ – Martin York Nov 20 '15 at 16:29
  • \$\begingroup\$ @Zulan: The problem with the visitor pattern is that if you add new types of objects that can be visited. This means that the interface ICartVisitor would need to be extended to those new types. But that also means that all classes that are derived from ICartVisitor must also be updated with the new type. This breaks the open/closed principle. It works really well if you don't need to update modify your types often (because then it just works and the open/closed principle does not come into play). \$\endgroup\$ – Martin York Nov 20 '15 at 19:10
5
\$\begingroup\$

First: you leak memory. Terribly. Never use new unless you know what you are doing. Use smart pointers, or in this case just objects on the stack.

The visitor pattern is meant for separating algorithms from an object structure. There is no object structure / composition / hierarchy in your code, so there is no use for the visitor pattern.

Following your example, there should be a Cart class with an accept method, that iteratively calls accept for all elements in the cart.

Also the visitor pattern is supposed to prepare for unknown algorithms operating on data structures that should not know about the algorithms. So it does not make sense to use a return value here. If you want to accumulate a prize here, you would have to do it in the state of the visitor.

Also: Use consistent spacing between type& variable etc. Use consistent capitalization of methods (visit). Avoid redundant newlines.

\$\endgroup\$
4
\$\begingroup\$

The visitor pattern allows for the implementation of external polymorphism and double dispatching.

Depending on where you are getting the design pattern documentation from, it may make for a bad implementation for C++.

The Visitor page on wikipedia for example, states you need a base class for the visitor, defining virtual entry points for the operations. This is true for Java for example, but not for C++.

For a more flexible visitor in C++, consider the use of functors instead of base visitor classes, not defining all visited classes in the same hierarchy, and using type erasure (through std::function) to hide away the differences in the functor types.

This provides you with a degree of flexibility that other languages usually do not allow for:

template<typename T> // CRTP: do not place all specializations in
                     // the same hierarchy
class Visited
{
public:

    // not needed:
    // using Visitor = std::function<void(IElement&)>;
    // using a template instead (suggested by @Deduplicator)

    // virtual is not mandatory, but this depends on your requirements
    template<typename Visitor>
    void visit(Visitor visitor)
    {
        visitor(*this);
    }
};

class Book: public Visited<Book>
{
public:
    virtual double price() { return 10; } // will be specialized for special books
};

class RareBook: public Book
{
    double price() override { return 100; }
};

class Pen: public BasicWritingTool, public Visited<Pen>
{
};

Book and Pen have different base classes.

// visitor 
struct GetPrice final
{
    double price;

    // book price + 10.5 %
    void operator()(Book& b) { price = b.price() * 110.5; } // b.price is virtual, so we
                                                            // have double dispatch
                                                            // for book specializations

    void operator()(Pen& p) { price = 10.5; } // price is computed differently for pens
};

Client code:

Book b{ "abcd" };
Pen p;
GetPrice priceCalculator;

b.visit(priceCalculator);
std::cout << "book price with our calculator: " << priceCalculator.price << "\n";

p.visit(priceCalculator);
std::cout << "pen price with our calculator: " << priceCalculator.price << "\n";
\$\endgroup\$
  • \$\begingroup\$ IElement should be T in your example, right? This is an interesting approach. I was tempted to suggest CRTP. \$\endgroup\$ – Zulan Nov 19 '15 at 15:31
  • 1
    \$\begingroup\$ I would avoid using std::function, using a template and allow any callable expecting compatible arguments is more flexible and performant. \$\endgroup\$ – Deduplicator Nov 19 '15 at 21:03
  • \$\begingroup\$ @Zulan, that is correct. \$\endgroup\$ – utnapistim Nov 19 '15 at 22:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.