4
\$\begingroup\$

Let's suppose there is a method of an Inputter class that queries many questions. The Human inputter would have to answer all those questions, but an AI inputter will do so without the questions being displayed to the screen. Some goes for command classes that contains the Inputter* as data member. We don't want separate methods for the two different types of inputters just because the AI will decide its own answers and does not need the questions displayed on the screen. So I've decided to use the Visitor Pattern to handle dealing with the two types of inputters.

#include <iostream>
#include <vector>
#include <ctime>

class Visitor;  class Human;  class AI;

class Inputter {
protected:
    std::string name;
public:
    Inputter (const std::string& n) : name(n) { }
    virtual ~Inputter() = default;
    std::string getName() const { return name; }
    void dine();
    virtual void accept (Visitor&) = 0;
};

class Visitor {
public:
    virtual void visit (Human*) = 0;
    virtual void visit (AI*) = 0;
};

class Human : public Inputter {
    using Inputter::Inputter;
    void accept (Visitor& visitor) override { visitor.visit(this); }
};

class AI : public Inputter {
    using Inputter::Inputter;
    void accept (Visitor& visitor) override { visitor.visit(this); }
};

// Use separate Visitor classes for each query to follow the open-closed principle.
class MealVisitor : public Visitor {
    const std::vector<std::string> mealOptions;
    std::string mealChosen;
public:
    MealVisitor (const std::vector<std::string>& m) : mealOptions(m) { }
    std::string getMealChosen() const { return mealChosen; }
private:
    void visit (Human*) override;
    void visit (AI*) override;
};

class TipVisitor : public Visitor {
    int tipGiven;
public:
    int getTipGiven() const { return tipGiven; }
private:
    void visit (Human*) override;
    void visit (AI*) override;
};

class Command {
protected:
    Inputter* inputter;
public:
    Command (Inputter* i) : inputter(i) { }
    virtual void execute() const = 0;
};

class PlayCommand : public Command {
    struct PlayCommandVisitor : Visitor {
        bool yes;
    };
    struct BreakVisitor : PlayCommandVisitor {
        void visit (Human*) override;
        void visit (AI*) override;
    };
    struct ResumeVisitor : PlayCommandVisitor {
        void visit (Human*) override;
        void visit (AI*) override;
    };
    using Command::Command;
public:
    void execute() const override;
private:
    template <typename VisitorType> bool getYesOrNo() const;
};

void Inputter::dine() {
    std::cout << name << " enters the restaurant.\n";
    MealVisitor mealVisitor({"steak", "spaghetti", "burger"});
    accept(mealVisitor);
    const std::string meal = mealVisitor.getMealChosen();
    std::cout << "20 minutes later, " << name << " has eaten the " << meal << ".\n";
    TipVisitor tipVisitor;
    accept(tipVisitor);
    const int tipGiven = tipVisitor.getTipGiven();
    std::cout << name << " gives " << "$" << tipGiven << " tip.\n\n";
}

void MealVisitor::visit (Human*) {
    int choiceNumber = 0;
    std::cout << "Choose a meal:\n";
    for (const std::string& meal : mealOptions)
        std::cout << ++choiceNumber << ". " << meal << '\n';
    while (true) {
        std::cin >> choiceNumber;
        if (1 <= choiceNumber && choiceNumber <= (int)mealOptions.size())
            break;
        std::cout << "Please choose one of the above options.\n";
    }
    mealChosen = mealOptions[choiceNumber - 1];
}

void MealVisitor::visit (AI*) {
    // No presentation with std::cout shall be given.
    mealChosen = mealOptions[std::rand() & mealOptions.size() - 1];
}

void TipVisitor::visit (Human* human) {
    std::cout << "How much will " << human->getName() << " tip?  \n";
    std::cin >> tipGiven;
}

void TipVisitor::visit (AI*) {
    // Do not query the AI.  Just let the AI do its part.
    tipGiven = std::rand() % 10 + 5;
}

void PlayCommand::execute() const {
    std::cout << inputter->getName() << " has been playing for 30 minutes.\n";
    const bool stop = getYesOrNo<BreakVisitor>();
    if (stop) {
        std::cout << inputter->getName() << " has decided to take a break from playing.\n";
        std::cout << inputter->getName() << " has taken a break for 10 minutes.\n";
        const bool resume = getYesOrNo<ResumeVisitor>();
        if (resume)
            std::cout << inputter->getName() << " resumes playing.\n";
        else
            std::cout << inputter->getName() << " does not want to play anymore.\n";    
    }
    else
        std::cout << inputter->getName() << " continues to play.\n";
}

template <typename VisitorType>
bool PlayCommand::getYesOrNo() const {
    VisitorType visitor;
    inputter->accept(visitor);
    return visitor.yes;
}

bool ask_yn (const std::string& question) {
    while (true) {
        std::cout << question << " (y/n) ";
        char yesNo;
        if (std::cin >> yesNo) {
            switch (yesNo) {
                case 'y': case 'Y': return true;
                case 'n': case 'N': return false;
            }
        }
        else {  // Input failed. Assume no.
            std::cout << '\n';
            return false;
        }
    }
}

void PlayCommand::BreakVisitor::visit (Human* human) {
    yes = ask_yn("Does " + human->getName() + " want to stop take a break?");
}

void PlayCommand::BreakVisitor::visit (AI*) {
    yes = std::rand() % 5;
}

void PlayCommand::ResumeVisitor::visit (Human* human) {
    yes = ask_yn("Does " + human->getName() + " want to resume playing?");
}

void PlayCommand::ResumeVisitor::visit (AI*) {
    yes = std::rand() % 2;
}

int main() {
    std::srand(std::time(nullptr));
    Human human("Bob");
    AI ai("Max");
    human.dine();
    ai.dine();
    
    const PlayCommand playCommand1(&human), playCommand2(&ai);
    playCommand1.execute();
    playCommand2.execute();
}

Sample output:

Bob enters the restaurant.
Choose a meal:
1. steak
2. spaghetti
3. burger
2
20 minutes later, Bob has eaten the spaghetti.
How much will Bob tip?
10
Bob gives $10 tip.

Max enters the restaurant.
20 minutes later, Max has eaten the steak.
Max gives $13 tip.

Bob has been playing for 30 minutes.
Does Bob want to stop take a break? (y/n) y
Bob has decided to take a break from playing.
Bob has taken a break for 10 minutes.
Does Bob want to resume playing? (y/n) n
Bob does not want to play anymore.
Max has been playing for 30 minutes.
Max has decided to take a break from playing.
Max has taken a break for 10 minutes.
Max resumes playing.
\$\endgroup\$
1
  • \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. In this case it would be better to create a followup question with a link back to this question. \$\endgroup\$
    – pacmaninbw
    Commented Jan 7, 2023 at 23:25

3 Answers 3

4
\$\begingroup\$

Use C++'s random number facilities

Avoid C's srand() and rand() functions, they are notoriously bad, and C++ has much better ways to deal with random numbers. Even if it doesn't really matter for the code you posted, it is better to teach yourself to do it correctly. See this example for a quick start.

Prefer to pass references instead of pointers

When you have to pass a pointer to a function and it should never be nullptr, pass a reference instead. So for example:

class Visitor {
public:
    virtual void visit (Human&) = 0;
    virtual void visit (AI&) = 0;
};

And:

class Command {
protected:
    Inputter& inputter;
public:
    Command (Inputter& i) : inputter(i) { }
    virtual void execute() const = 0;
};

Do you need the visitor pattern?

Given your history of questions on Code Review, I sense a pattern (pun intended): you are trying to write code that excercises each design pattern out there. While it might be good to know about design patterns so you know how to implement them when you really need them, you should not contort your code to use a certain pattern if it doesn't need it, otherwise you're doing the equivalent of using a hammer to put in screws.

Consider that instead of creating a Visitor class, and deriving a MealVisitor and TipVisitor from it, you could just add virtual member functions chooseMeal() and chooseTip() to Inputter, which Human and AI could implement however they want. So then you have code like:

void Inputter::dine() {
    std::cout << name << " enters the restaurant.\n";
    const std::string meal = chooseMeal({"steak", "spaghetti", "burger"});
    std::cout << "20 minutes later, " << name << " has eaten the " << meal << ".\n";
    const int tipGiven = chooseTip();
    std::cout << name << " gives " << "$" << tipGiven << " tip.\n\n";
}
…
std::string Human::chooseMeal(const std::vector<std::string>& mealOptions) {
    std::size_t choiceNumber = 0;
    …
    return mealOptions[choiceNumber - 1];
}

std::string AI::chooseMeal(const std::vector<std::string>& mealOptions) {
    return mealOptions[std::rand() & mealOptions.size() - 1];
}

Note how dine() is now much more compact, and you didn't have to create Visitor and derived classes. So the visitor pattern was overkill here and just made the code needlessly complicated.

Of course the visitor pattern is useful, and its greatest strength is that it allows you to remove the implementation of chooseMeal() and chooseTip() from Human and AI, making those classes simpler. It allows you to add behavior later, like choosing a payment method, without having to modify Inputter and its derived classes. But at that point you should ask yourself: should dine() even be a member function of Inputter? Why not create a DineVisitor?

Inheritance vs. generic programming

Instead of using inheritance and virtual member functions, you can use generic programming in C++ to get similar results. For example, you can write:

template<typename T>
void dine(T& inputter) { 
    std::cout << inputter.name << " enters the restaurant.\n";
    const std::string meal = chooseMeal(inputter, {"steak", "spaghetti", "burger"});
    …
}

std::string chooseMeal(Human&, const std::vector<std::string>& mealOptions) {
    std::size_t choiceNumber = 0;
    …
    return mealOptions[choiceNumber - 1];
}

std::string chooseMeal(AI&, const std::vector<std::string>& mealOptions) {
    return mealOptions[std::rand() & mealOptions.size()];
}

And then write:

Human human("Bob");
AI ai("Max");
dine(human);
dine(ai);

Whereas with your code, dine() only needed to know about the base class, the problem here is that dine() only works on the derived classes. If you want to create a vector of Inputters, then since C++17 you can use std::variant, and write:

std::vector<std::variant<Human, AI>> inputters = {
    Human("Bob"),
    AI("Max"),
    Human("Alice"),
    …
};

for (auto& inputter: inputters) {
    std::visit(dine, inputter);
}

Note that Human and AI don't need to derive from any base class when using generics.

Avoid casting where possible

It is better to avoid casting, as incorrect casting might cause subtle problems. While it doesn't matter in your code, it is better to do it correctly so you learn the correct habit, and don't make a mistake when it does matter.

In particular, you are casting mealOptions.size() to int using a C-style cast. You should already prefer to use static_cast<>() here, as it is safer than C-style casts, but it is even better to avoid it entirely by making choiceNumber a std::size_t instead.

Review of your update

As for the version of PlayCommand that uses std::variant, you can make a std::variant of references, but you have to use std::reference_wrapper. But you don't need std::variant in the visitor itself, see below.

You can avoid storing both a reference to Inputter and a variant by using std::visit() to call .name() on the inputter, or just one that returns a pointer to the base class.

If you are learning design patterns from a book that uses Java to show how to implement the patterns, they have to work around some limitations of that language. In C++ you can have free functions, so instead of trying to create a class hierarchy, consider creating a stand-alone function instead:

static bool queryBreak(Human& human) {
    return ask_yn("Does " + human.getName() + " want to stop take a break?");
}

static bool queryBreak(AI&) {
        return std::rand() % 2;
}
…
void play(auto& inputter) {
    std::cout << inputter.getName() << " has been playing for 30 minutes.\n";
    const bool stop = queryBreak(inputter);
    if (stop) {
        std::cout << inputter.getName() << " has decided to take a break from playing.\n";
        std::cout << inputter.getName() << " has taken a break for 10 minutes.\n";
        const bool resume = queryResume(inputter);
        if (resume)
            std::cout << inputter.getName() << " resumes playing.\n";
        else
            std::cout << inputter.getName() << " does not want to play anymore.\n"; 
    }
    else
        std::cout << inputter.getName() << " continues to play.\n";
}

Note how play() still fits the definition of a visitor. One drawback is that play() is now a template. This can be avoided by passing it a variant of references or pointers as a parameter. To get a pointer to Inputter, you can use a visitor. I also show here how to use std::reference_wrapper:

using InputterReferenceVariant = std::variant<
    std::reference_wrapper<Human>,
    std::reference_wrapper<AI>
>;

void play(InputterReferenceVariant inputterVariant) {
    Inputter& inputter = std::visit([](auto& v) -> Inputter& {
        return v;
    }, inputterVariant);
    std::cout << inputter.getName() << " has been playing for 30 minutes.\n";
    const bool stop = std::visit(queryBreak, inputterVariant);
    …
}
…
Human human("Bob");
AI ai("Max");
play(human);
play(ai);
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for all the great reviews. I've appended my post with a revised Command class using std::variant (perhaps you can check it out), which I think had to use pointers for its types because the overloads used with std::visit would not work otherwise. And yes, I am trying to master every design pattern so that I can detect the need for one when the moment is right (but in most cases, one has to make up his own patterns to fit his needs, which I think you have been saying all along). \$\endgroup\$
    – prestokeys
    Commented Jan 7, 2023 at 20:28
  • \$\begingroup\$ Rats. They deleted my update. So I posted my modified PlayCommand class as an answer. \$\endgroup\$
    – prestokeys
    Commented Jan 8, 2023 at 0:40
  • \$\begingroup\$ @ G. Sliepen Thanks again. I've updated my answer below to handle the case if the AI aspect of Inputter does not depend on its derived type but on a AI_type data member. It is much, much shorter now and follows your ideas. I'll take it as good if you don't say anything. ;) \$\endgroup\$
    – prestokeys
    Commented Jan 9, 2023 at 1:51
1
\$\begingroup\$

Commenting on one thing I didn't see the other answers include.

Guard Clauses

Instead of

if (condition) {
    // return code
} else {
    // rest of function
}

You should do

if (!condition) {
    // rest of function
}
// return code

It reduces a lot of unnecessary indentation, and overall makes your code more readable.

This video, specifically 1:00 - 4:03, has a great explanation on why this is something you should factor in to your programming.

\$\endgroup\$
0
\$\begingroup\$

Using G.Sliepen's advice, though his example could not compile, I used std::variant and std::visit (which is very picky in the form of its first argument) and changed my Command classes to:

template <typename... Ts>
struct Overload : Ts... {
    using Ts::operator()...;
};

template <class... Ts> Overload(Ts...) -> Overload<Ts...>;

class Command {
protected:
    Inputter& inputter;
    std::variant<Human*, AI*> inputterVariant;  // std::variant disallows using it with references (if using values instead, the overloads with references in the std::visit usage will not work).
public:
    Command (Human& human) : inputter(human), inputterVariant(&human) { }
    Command (AI& ai) : inputter(ai), inputterVariant(&ai) { }
    virtual void execute() const = 0;
};

class PlayCommand : public Command {
    using Command::Command;
public:
    void execute() const override;
private:
    static constexpr auto queryBreak = Overload {
        [](Human* human)->bool { return askYesNo("Does " + human->getName() + " want to stop take a break?"); },
        [](AI*)->bool { return std::rand() % 2; },
    },
    queryResume = Overload {
        [](Human* human)->bool { return askYesNo("Does " + human->getName() + " want to resume playing?"); },
        [](AI*)->bool { return std::rand() % 3; },
    };
};

void PlayCommand::execute() const {
    std::cout << inputter.getName() << " has been playing for 30 minutes.\n";  // 'inputter' must be used for the generic case.  Using 'std::get<Human*>(inputterVariant)' will only crash when the variant type is AI*. 
    const bool stop = std::visit(queryBreak, inputterVariant);  // 'queryBreak' could be replaced with its definition directly inside the std::visit call, but to reduce the clutter it has been defined privately within the PlayCommand class.  Similarly for 'queryResume'.
    if (stop) {
        std::cout << inputter.getName() << " has decided to take a break from playing.\n";
        std::cout << inputter.getName() << " has taken a break for 10 minutes.\n";
        const bool resume = std::visit(queryResume, inputterVariant);
        if (resume)
            std::cout << inputter.getName() << " resumes playing.\n";
        else
            std::cout << inputter.getName() << " does not want to play anymore.\n"; 
    }
    else
        std::cout << inputter.getName() << " continues to play.\n";
}

This tested out to work correctly. When I first tried using

std::variant<Human, AI> inputterVariant

the overloads

queryBreak = Overload {
        [](Human& human)->bool { return askYesNo("Does " + human.getName() + " want to stop take a break?"); },
        [](AI&)->bool { return std::rand() % 2; },
    },
queryResume = Overload {
        [](Human& human)->bool { return askYesNo("Does " + human->getName() + " want to resume playing?"); },
        [](AI&)->bool { return std::rand() % 3; },
    };

did not compile. Changing the overload arguments from reference to value did compile, but the extra copy constructor calls would cost some performance, so I decided to go with pointers again.

Update:

What if the AI aspect of Inputter does not depend on its derived type but on a AI_Type data member (which includes the NoAI case)? Here is a very short, fully compiling code to handle that, using G.Sliepen's suggestion to avoid the Visitor Pattern (there's no overload here anymore anyway) and just do what comes natural:

#include <iostream>
#include <vector>
#include <array>
#include <functional>
#include <ctime>

enum AI_Type { NoAI, StandardAI, num_AI_types };  // Later, there could be more, e.g. CraftyAI, LazyAI, etc...

class Inputter {
    std::string name;
    AI_Type aiType;
public:
    Inputter (const std::string& n, AI_Type a = StandardAI) : name(n), aiType(a) { }
    void dine();
};

void Inputter::dine() {
    const std::vector<std::string> mealOptions = {"steak", "spaghetti", "burger"};
    // Helper functions declared here instead of altering the Inputter class, since they don't use overload.
    static const std::array<std::function<std::string()>, num_AI_types> chooseMeal = {
        // Component 0 is NoAI.
        [&]() {
            std::size_t choiceNumber = 0;
            std::cout << "Choose a meal:\n";
            for (const std::string& meal : mealOptions)
                std::cout << ++choiceNumber << ". " << meal << '\n';
            while (true) {
                std::cin >> choiceNumber;
                if (1 <= choiceNumber && choiceNumber <= mealOptions.size())
                    break;
                std::cout << "Please choose one of the above options.\n";
            }
            return mealOptions[choiceNumber - 1];
        },
        // Component 1 is StandardAI.
        [&]() { return mealOptions[std::rand() % mealOptions.size()]; }
    };
    static const std::array<std::function<int()>, num_AI_types> chooseTip = {
        [this]() {
            int tip;
            std::cout << "How much will " << name << " tip?  \n";
            std::cin >> tip;
            return tip;
        },
        []() { return std::rand() % 10 + 5; }
    };
    std::cout << name << " enters the restaurant.\n";
    const std::string meal = chooseMeal[aiType]();
    std::cout << "20 minutes later, " << name << " has eaten the " << meal << ".\n";
    const int tip = chooseTip[aiType]();
    std::cout << name << " gives " << "$" << tip << " tip.\n\n";
}

int main() {
    std::srand(std::time(nullptr));
    Inputter human("Bob", NoAI), ai("Max");
    human.dine();
    ai.dine();
}

The output is just as before.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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