3
\$\begingroup\$

My code below ensures that only land animals can be a "representatives" for Land while only ocean animals can be representatives for Ocean, and so forth for Air, Underground, etc... Just want some feedback about whether this system I've come up with is any good or if there is a better one. Note: I'm using structs instead of classes to keep it simple.

#include <iostream>
#include <list>

struct IAnimal { virtual ~IAnimal() = default; };

template <typename T>
struct Animal : virtual IAnimal {};  // Animal<T> can only be pair with T, where T is derived from Place.

struct Place {
    IAnimal* representative;
    virtual bool setRepresentative (IAnimal* a) {
        representative = a;  std::cout << "Done.\n";  return true;
    }
    virtual bool canUse (const IAnimal*) const = 0;
};

template <typename Derived>
struct PlaceCRTP : Place {
    virtual bool setRepresentative (IAnimal* a) override {
        if (canUse(a))
            return Place::setRepresentative(a);
        std::cout << "Cannot be done.\n";
        return false;
    }
    virtual bool canUse (const IAnimal* a) const override {
        return dynamic_cast<const Animal<Derived>*>(a) != nullptr;
    }
};

struct Land : PlaceCRTP<Land> {};
struct Ocean : PlaceCRTP<Ocean> {};

struct Elephant : Animal<Land> {};
struct Moose : Animal<Land> {};
struct Deer : Animal<Land> {};
struct Whale : Animal<Ocean> {};
struct Shark : Animal<Ocean> {};
struct Crab : Animal<Ocean>, Animal<Land> {};  // Note: Crab can represent Ocean or Land!

int main() {
    Land land;
    Elephant elephant;
    Whale whale;
    land.setRepresentative(&elephant);  // Done.
    land.setRepresentative(&whale);  // Cannot be done.

    std::list<IAnimal*> allAnimals, landCandidates;
    for (int i = 0; i < 10; i++) {
        allAnimals.push_back (new Elephant);
        allAnimals.push_back (new Whale);
        allAnimals.push_back (new Moose);
        allAnimals.push_back (new Shark);
        allAnimals.push_back (new Crab);
    }
    for (IAnimal* a : allAnimals)
        if (land.canUse(a))
            landCandidates.push_back(a);
    std::cout << allAnimals.size() << std::endl;  // 50
    std::cout << landCandidates.size() << std::endl;  // 30 (10 Elephants, 10 Moose, 10 Crabs)
}

One drawback I can think of right away, is that the repeated dynamic casts cost performance, but looking up a map might be slower.

\$\endgroup\$
  • 2
    \$\begingroup\$ I have rolled back your revisions to your question. Etiquette here on Code Review suggests two things: Changes you make to your code that are inspired by answers to your quesiton, should be presented for review in a new question (What you may and may not do after receiving answers). Secondly, it is polite to upvote, and accept those answers that most help you with your concerns. \$\endgroup\$ – rolfl Jan 28 '15 at 20:48
2
\$\begingroup\$

The design of Place.canUse(Animal) suggests that places use animals. But in reality it's the other way around. The question is not if land can use a whale, but if a whale can "use" land. I suggest to invert the design to Animal.canUse(Place) (or possibly even canInhabit).

If performance is important, then you can add in Animal one method for each Place type, for example canUseLand, canUseOcean. After all, do you seriously need to consider future expansion of place types? It's the animal kingdom that's likely to grow. And the canUseX methods only need to be implemented in higher level classes, the vast majority will be happy to inherit.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. You've inspired me to write another solution using double dispatch instead of CRTP (updated in my question). \$\endgroup\$ – prestokeys Jan 24 '15 at 15:01
  • \$\begingroup\$ That belongs to another question... \$\endgroup\$ – janos Jan 24 '15 at 16:06

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.