Visitor design pattern used for an asteroid game

Question

Please review my code below. I am studying the Visitor Design Pattern and I think I have implemented it correctly. Please check. I have tried to split the method definitions and the prototypes as much as possible so there are many files. Any comments are welcome. And no, this is not homework.

AsteroidBase.cpp

#include <iostream>

#include "AsteroidBase.h"
#include "SpaceShipBase.h"

CAsteroidBase::CAsteroidBase(const std::string& aAsteroidName) : iAsteroidName(aAsteroidName)
{
}

AsteroidBase.h

#ifndef ASTEROIDBASE_H
#define ASTEROIDBASE_H

class CSpaceShipBase;
class CApolloSpaceShip;
class CColumbiaSpaceShip;

class CAsteroidBase
{
public:
  CAsteroidBase(const std::string& aAsteroidName);
  CAsteroidBase(CAsteroidBase& aAsteroidBase) = default;
  CAsteroidBase(CAsteroidBase&& aAsteroidBase) = default;
  ~CAsteroidBase() = default;
  CAsteroidBase& operator=(CAsteroidBase& aAsteroidBase) = default;
  CAsteroidBase& operator=(CAsteroidBase&& aAsteroidBase) = default;

  virtual const std::string& GetAsteroidName() const = 0;

protected:
  std::string iAsteroidName;
};

#endif // ASTEROIDBASE_H

SpaceShipBase.h

#ifndef SPACESHIPBASE_H
#define SPACESHIPBASE_H

#include <memory>

class CAsteroidBase;
class CExplodingAsteroid;
class CStonyAsteroid;

class CSpaceShipBase
{
public:
  CSpaceShipBase(const std::string& aSpaceShipName);
  CSpaceShipBase(const CSpaceShipBase& aSpaceShipBase) = default;
  CSpaceShipBase(CSpaceShipBase&& aSpaceShipBase) = default;

  virtual ~CSpaceShipBase() = default;

  CSpaceShipBase& operator=(CSpaceShipBase& aSpaceShipBase) = default;
  CSpaceShipBase& operator=(CSpaceShipBase&& aSpaceShipBase) = default;

  virtual const std::string& GetSpaceShipName() const = 0;

  virtual void ShipCollidesWith(std::shared_ptr<CAsteroidBase> aAsteroidBase) const = 0;
  virtual void ShipCollidesWith(std::shared_ptr<CExplodingAsteroid> aExplodingAsteroid) const = 0;
  virtual void ShipCollidesWith(std::shared_ptr<CStonyAsteroid> aStonyAsteroid) const = 0;

protected:
  std::string iSpaceShipName;
};

#endif // SPACESHIPBASE_H

SpaceShipbase.cpp

#include <iostream>

#include "SpaceShipBase.h"
#include "AsteroidBase.h"

CSpaceShipBase::CSpaceShipBase(const std::string& aSpaceShipName) : iSpaceShipName(aSpaceShipName)
{
}

ApolloSpaceShip.cpp

#include <iostream>

#include "ApolloSpaceShip.h"
#include "AsteroidBase.h"
#include "ExplodingAsteroid.h"
#include "StonyAsteroid.h"

CApolloSpaceShip::CApolloSpaceShip(const std::string& aSpaceShipName) : CSpaceShipBase(aSpaceShipName)
{
}

const std::string& CApolloSpaceShip::GetSpaceShipName() const
{
  return iSpaceShipName;
}

void CApolloSpaceShip::ShipCollidesWith(std::shared_ptr<CExplodingAsteroid> aExplodingAsteroid) const
{
  std::cout << __PRETTY_FUNCTION__ << ": " << GetSpaceShipName() << " and " << aExplodingAsteroid->GetAsteroidName()
    << " collide." << std::endl;
}

void CApolloSpaceShip::ShipCollidesWith(std::shared_ptr<CStonyAsteroid> aStonyAsteroid) const
{
  std::cout << __PRETTY_FUNCTION__ << ": " << GetSpaceShipName() << " and " << aStonyAsteroid->GetAsteroidName()
    << " collide." << std::endl;
}

void CApolloSpaceShip::ShipCollidesWith(std::shared_ptr<CAsteroidBase> aAsteroidBase) const
{
  std::cout << __PRETTY_FUNCTION__ << ": " << GetSpaceShipName() << " and " << aAsteroidBase->GetAsteroidName()
    << " collide." << std::endl;
}

ApolloSpaceShip.h

#ifndef APOLLOSPACESHIP_H
#define APOLLOSPACESHIP_H

#include "SpaceShipBase.h"

class CAsteroidBase;
class CExplodingAsteroid;
class CStonyAsteroid;

class CApolloSpaceShip : public CSpaceShipBase
{
public:
  CApolloSpaceShip(const std::string& aSpaceShipName);
  CApolloSpaceShip(CApolloSpaceShip& aApolloSpaceShip) = default;
  CApolloSpaceShip(CApolloSpaceShip&& aApolloSpaceShip) = default;

  ~CApolloSpaceShip() = default;
  CApolloSpaceShip& operator=(CApolloSpaceShip& aApolloSpaceShip) = default;
  CApolloSpaceShip& operator=(CApolloSpaceShip&& aApolloSpaceShip) = default;

  const std::string& GetSpaceShipName() const override;

  void ShipCollidesWith(std::shared_ptr<CAsteroidBase> aAsteroidBase) const override;
  void ShipCollidesWith(std::shared_ptr<CExplodingAsteroid> aExplodingAsteroid) const override;
  void ShipCollidesWith(std::shared_ptr<CStonyAsteroid> aStonyAsteroid) const override;
};

#endif // APOLLOSPACESHIP_H

ColumbiaSpaceShip.cpp

#include <iostream>

#include "ColumbiaSpaceShip.h"
#include "AsteroidBase.h"
#include "ExplodingAsteroid.h"
#include "StonyAsteroid.h"

CColumbiaSpaceShip::CColumbiaSpaceShip(const std::string& aSpaceShipName) : CSpaceShipBase(aSpaceShipName)
{
}

const std::string& CColumbiaSpaceShip::GetSpaceShipName() const
{
  return iSpaceShipName;
}

void CColumbiaSpaceShip::ShipCollidesWith(std::shared_ptr<CExplodingAsteroid> aExplodingAsteroid) const
{
  std::cout << __PRETTY_FUNCTION__ << ": " << GetSpaceShipName() << " and " << aExplodingAsteroid->GetAsteroidName()
    << " collide." << std::endl;
}

void CColumbiaSpaceShip::ShipCollidesWith(std::shared_ptr<CStonyAsteroid> aStonyAsteroid) const
{
  std::cout << __PRETTY_FUNCTION__ << ": " << GetSpaceShipName() << " and " << aStonyAsteroid->GetAsteroidName()
    << " collide." << std::endl;
}

void CColumbiaSpaceShip::ShipCollidesWith(std::shared_ptr<CAsteroidBase> aAsteroidBase) const
{
  std::cout << __PRETTY_FUNCTION__ << ": " << GetSpaceShipName() << " and " << aAsteroidBase->GetAsteroidName()
    << " collide." << std::endl;
}

ColumbiaSpaceShip.h

#ifndef COLUMBIASPACESHIP_H
#define COLUMBIASPACESHIP_H

#include "SpaceShipBase.h"

class CAsteroidBase;
class CExplodingAsteroid;
class CStonyAsteroid;

class CColumbiaSpaceShip : public CSpaceShipBase
{
public:
  CColumbiaSpaceShip(const std::string& aSpaceShipName);
  CColumbiaSpaceShip(CColumbiaSpaceShip& aColumbiaSpaceShip) = default;
  CColumbiaSpaceShip(CColumbiaSpaceShip&& aColumbiaSpaceShip) = default;
  ~CColumbiaSpaceShip() = default;
  CColumbiaSpaceShip& operator=(CColumbiaSpaceShip& aColumbiaSpaceShip) = default;
  CColumbiaSpaceShip& operator=(CColumbiaSpaceShip&& aColumbiaSpaceShip) = default;

  const std::string& GetSpaceShipName() const override;

  void ShipCollidesWith(std::shared_ptr<CAsteroidBase> aAsteroidBase) const override;
  void ShipCollidesWith(std::shared_ptr<CExplodingAsteroid> aExplodingAsteroid) const override;
  void ShipCollidesWith(std::shared_ptr<CStonyAsteroid> aStonyAsteroid) const override;
};

#endif // COLUMBIASPACESHIP_H

ExplodingAsteroid.cpp

#include <iostream>

#include "ExplodingAsteroid.h"

CExplodingAsteroid::CExplodingAsteroid(const std::string& aAsteroidName) : CAsteroidBase(aAsteroidName)
{
}

const std::string& CExplodingAsteroid::GetAsteroidName() const
{
  return iAsteroidName;
}

ExplodingAsteroid.h

#ifndef EXPLODINGASTEROID_H
#define EXPLODINGASTEROID_H

#include "AsteroidBase.h"

class CExplodingAsteroid : public CAsteroidBase
{
public:
  CExplodingAsteroid(const std::string& aAsteroidName);
  CExplodingAsteroid(CExplodingAsteroid& aExplodingAsteroid) = default;
  CExplodingAsteroid(CExplodingAsteroid&& aExplodingAsteroid) = default;
  ~CExplodingAsteroid() = default;
  CExplodingAsteroid& operator=(CExplodingAsteroid& aExplodingAsteroid) = default;
  CExplodingAsteroid& operator=(CExplodingAsteroid&& aExplodingAsteroid) = default;


  const std::string& GetAsteroidName() const override;
};

#endif // EXPLODINGASTEROID_H

StonyAsteroid.cpp

#include <iostream>

#include "StonyAsteroid.h"

CStonyAsteroid::CStonyAsteroid(const std::string& aAsteroidName) : CAsteroidBase(aAsteroidName)
{
}

const std::string& CStonyAsteroid::GetAsteroidName() const
{
  return iAsteroidName;
}

StonyAsteroid.h

#ifndef STONYASTEROID_H
#define STONYASTEROID_H

#include "AsteroidBase.h"

class CStonyAsteroid : public CAsteroidBase
{
public:
  CStonyAsteroid(const std::string& aAsteroidName);
  CStonyAsteroid(CStonyAsteroid& aStonyAsteroid) = default;
  CStonyAsteroid(CStonyAsteroid&& aStonyAsteroid) = default;

  virtual ~CStonyAsteroid() = default;

  CStonyAsteroid& operator=(CStonyAsteroid& aStonyAsteroid) = default;
  CStonyAsteroid& operator=(CStonyAsteroid&& aStonyAsteroid) = default;

  const std::string& GetAsteroidName() const override;
};

#endif // STONYASTEROID_H

main.cpp

#include <vector>
#include <iostream>
#include <memory>

#include "AsteroidBase.h"
#include "SpaceShipBase.h"
#include "ApolloSpaceShip.h"
#include "ColumbiaSpaceShip.h"

#include "ExplodingAsteroid.h"
#include "StonyAsteroid.h"

typedef std::vector<std::shared_ptr<CSpaceShipBase>> TSpaceShipBaseVec;
typedef std::vector<std::shared_ptr<CAsteroidBase>>  TAsteroidBaseVec;

TSpaceShipBaseVec CreateShipVec()
{
  std::shared_ptr<CSpaceShipBase> spaceShip_1 = std::make_shared<CApolloSpaceShip>("Apollo 6");
  std::shared_ptr<CSpaceShipBase> spaceShip_2 = std::make_shared<CApolloSpaceShip>("Apollo 13");
  std::shared_ptr<CSpaceShipBase> spaceShip_3 = std::make_shared<CApolloSpaceShip>("Apollo 17");
  std::shared_ptr<CSpaceShipBase> spaceShip_4 = std::make_shared<CColumbiaSpaceShip>("Columbia");

  TSpaceShipBaseVec spaceShipBaseVec;

  spaceShipBaseVec.push_back(spaceShip_1);
  spaceShipBaseVec.push_back(spaceShip_2);
  spaceShipBaseVec.push_back(spaceShip_3);
  spaceShipBaseVec.push_back(spaceShip_4);

  return spaceShipBaseVec;
}

TAsteroidBaseVec CreateAsteroidBaseVec()
{
  std::shared_ptr<CAsteroidBase> asteroid_1 = std::make_shared<CExplodingAsteroid>("exploding asteroid");
  std::shared_ptr<CAsteroidBase> asteroid_2 = std::make_shared<CStonyAsteroid>("stony asteroid");

  TAsteroidBaseVec asteroidBaseVec;

  asteroidBaseVec.push_back(asteroid_1);
  asteroidBaseVec.push_back(asteroid_2);

  return asteroidBaseVec;
}

int main()
{
  TSpaceShipBaseVec spaceShipBaseVec = CreateShipVec();
  TAsteroidBaseVec  asteroidBaseVec  = CreateAsteroidBaseVec();

  for(std::shared_ptr<CSpaceShipBase> shipBasePtr : spaceShipBaseVec)
  {
    for(std::shared_ptr<CAsteroidBase> asteroidBasePtr : asteroidBaseVec)
    {
      shipBasePtr->ShipCollidesWith(asteroidBasePtr);
    }
  }

  return 0;
}

Answer 1

Visitor

A visitor pattern would look more like this:

class Visitor;
class Asteroid
{
    public:
        virtual ~Asteroid();
        virtual void accept(Visitor&) = 0; // The asteroid accepts visitor
};
class ExplodingAsteroid;
class StonyAsteroid;
class Visitor
{
     public:
         // Limitation of the visitor that it must know about
         // about all the classes it can visit at compile time.
         virtual void hitsAsteroidOfType(ExplodingAsteroid&) = 0;
         virtual void hitsAsteroidOfType(StonyAsteroid&)     = 0;
};

// Class's that implement the Visitor pattern
// Simply call the appropriate method on the visitor that 
// they need to to complete there task. In this case there
// is only one method: `hitsAsteroidWith()`
class ExplodingAsteroid: public Asteroid
{
    public:
        virtual void accept(Visitor& v) override {
            v.hitsAsteroidOfType(*this);
        }
};
class StonyAsteroid: public Asteroid
{
    public:
        virtual void accept(Visitor& v) override {
            v.hitsAsteroidOfType(*this);
        }
};


class LokiClassStarDestroyer: public Visitor
{
     public:
         // Limitation of the visitor that it must know about
         // about all the classes it can visit at compile time.
         void hitsAsteroidOfType(ExplodingAsteroid&) override {
             std::cout << "Hit an Exploding Asteroid (hardly notices the rumble)\n";
         }
         void hitsAsteroidWith(StonyAsteroid&) override {
             std::cout << "Hit an Stony Asteroid (Incorporates material in to ablative shields)\n";
         }
};

Doing a quick code review:

My first complaint is that I always want to see the interface before I see the implementation. As there are so many details I need before the implementation makes sense. So put header before the source file.

A second complain is that the name of the class does not match the name of the file. Yes you simply prefix each class name with 'C'. Personally I hate this convention but others use it so can't really complain. But be consistent with filename and class name.

Last complaint is that you don't use namespace. You should put all your code inside your own namespace. It will avoid issues later when you and your future collaborators have used the same class name accidentally. Also by include the namespace in your include guards you help make sure that they are unique.

AsteroidBase.h

These classes are not used in this header.

class CSpaceShipBase;
class CApolloSpaceShip;
class CColumbiaSpaceShip;

Remove them.

If all these standard methods are not defined then you don't need to ask the compiler to generate them. They will be generated automatically. You only need to include these when a method would be not defined because others are defined.

  CAsteroidBase(CAsteroidBase& aAsteroidBase) = default;
  CAsteroidBase(CAsteroidBase&& aAsteroidBase) = default;
  ~CAsteroidBase() = default;
  CAsteroidBase& operator=(CAsteroidBase& aAsteroidBase) = default;
  CAsteroidBase& operator=(CAsteroidBase&& aAsteroidBase) = default;

This is not really necessary. All your sub-classes just return iAsteroidName. So just return it here so you don't need to have the same default implementation in all sub-classes.

  virtual const std::string& GetAsteroidName() const = 0;

When you have protected it usually means a bad design. Re-look at your code and make sure you absolutely need it.

protected:

AsteroidBase.cpp

I don't see you using iostream operations in this class.

#include <iostream>

Remove it.

Why are you include SpaceShipBase.h here?

#include "SpaceShipBase.h"

Again. Not used in this class.

If this is the only method in this class. And it is this simple. There is an argument for including it as part of the class declaration (so you don't need the cpp file.

CAsteroidBase::CAsteroidBase(const std::string& aAsteroidName) : iAsteroidName(aAsteroidName)
{
}

To avoid the line being to long I put the initializer list on the next few rows (indented). This will prevent lines from getting to long. Also with more members you want to initialize one per line.

// I would do it like this:
AsteroidBase::AsteroidBase(std::string const& aAsteroidName)
    : iAsteroidName(aAsteroidName)
{}

SpaceShipBase.h

Again you don't need to say default here.

  CSpaceShipBase(const CSpaceShipBase& aSpaceShipBase) = default;
  CSpaceShipBase(CSpaceShipBase&& aSpaceShipBase) = default;

  virtual ~CSpaceShipBase() = default;

  CSpaceShipBase& operator=(CSpaceShipBase& aSpaceShipBase) = default;
  CSpaceShipBase& operator=(CSpaceShipBase&& aSpaceShipBase) = default;

I presume this is your visitor part.

  virtual void ShipCollidesWith(std::shared_ptr<CAsteroidBase> aAsteroidBase) const = 0;
  virtual void ShipCollidesWith(std::shared_ptr<CExplodingAsteroid> aExplodingAsteroid) const = 0;
  virtual void ShipCollidesWith(std::shared_ptr<CStonyAsteroid> aStonyAsteroid) const = 0;

This looks good. Though I would not pass the std::shared_ptr<> part. Just pass the class as a reference. The visitor does not need to know how you are maintaining ownership. So something like this:

  virtual void ShipCollidesWith(CAsteroidBase& aAsteroidBase) const = 0;
  virtual void ShipCollidesWith(CExplodingAsteroid& aExplodingAsteroid) const = 0;
  virtual void ShipCollidesWith(CStonyAsteroid& aStonyAsteroid) const = 0;

SpaceShipbase.cpp

Don't see the need for this:

#include "AsteroidBase.h"

Again this is so simple that I would just include it as part of the class declaration rather than needing a whole file for itself.

CSpaceShipBase::CSpaceShipBase(const std::string& aSpaceShipName) : iSpaceShipName(aSpaceShipName)
{
}

Answer 2

All spaceships calls the same function:

ShipCollidesWith(std::shared_ptr<CAsteroidBase>)

the functions with more concrete Asteroid types are not called:

void ShipCollidesWith(std::shared_ptr<CExplodingAsteroid> aExplodingAsteroid) const override;
void ShipCollidesWith(std::shared_ptr<CStonyAsteroid> aStonyAsteroid) const override;

So these two functions are not used at all.

The point of visitor pattern is: given a base pointer, able to pass it to a function defined with concrete derive type parameter.

Answer 3

No, this is not a visitor pattern. You are trying to do double dispatch. You are actually calling void ShipCollidesWith(std::shared_ptr<CAsteroidBase> aAsteroidBase) every time. There is nothing I see that will then do the second dispatch to the right derived class for the actual type of the parameter.

What did your test program print??

---

There are other problems with the code: the copy constructor’s lvalue reference parameter should be const. Ditto for assignment.

Passing the shared_ptr by value is inefficient and very slow, since it has to do the thread-safe increment and decrement of the counter. Pass by reference, except where you are going to keep a copy beyond the lifetime of the function.

Use auto, especially when you are naming the type twice now.

---