Object oriented design suggestion

Question

Here is my code:

class Soldier {
public:
   Soldier(const string &name, const Gun &gun);
   string getName();
private:
   Gun gun;
   string name;
};

class Gun {
public:
   void fire();
   void load(int bullets);
   int getBullets();
private:
   int bullets;
}

I need to call all the member functions of Gun over a Soldier object. Something like:

soldier.gun.fire();

or

soldier.getGun().load(15);

So which one is a better design? Hiding the gun object as a private member and access it with getGun() function. Or making it a public member? Or I can encapsulate all these functions would make the implementation harder:

soldier.loadGun(15); // calls Gun.load()
soldier.fire(); // calls Gun.fire()

So which one do you think is the best?

Answer 1

I would say go with your second option:

soldier.loadGun(15); // calls Gun.load()
soldier.fire(); // calls Gun.fire()

Initially it's more work, but as the system gets more complex, you may find that a soldier will want to do other things before and after firing their gun (maybe check if they have enough ammo and then scream "Die suckers!!" before firing, and mutter "that's gotta hurt" after, and check to see if they need a reload). It also hides from the users of the Soldier class the unnecessary details of how exactly the gun is being fired.

Answer 2

Usually my decision is based on the nature of the container class (in this case, Soldier). Either it is entirely a POD or is not. If it's not a POD, I make all data members private and provide accessor methods. The class is a POD only if it has no invariants (i.e. there is no way an external actor can make its state inconsistent by modifying its members). Your soldier class looks more like a non-POD to me, so I would go to the accessor method option. If it would return a const reference or a regular reference is your own decision, based on the behaviour of fire() and the other methods (if they modify gun's state or not).

BTW, Bjarne Stroustrup talks a little about this issue in his site: http://www.artima.com/intv/goldilocks3.html

A sidenote: I know that's not precisely what you asked, but I'd advice you to also consider the many mentions made in other answers to the law of Demeter: to expose action methods (that act on gun) instead of the entire gun object via a getter method. Since the soldier "has" the gun (it is in his hand and he pulls the trigger), it seems more natural to me that the other actors "ask" the soldier to fire. I know this may be tedious if gun has many methods to act on, but maybe also these could be grouped in more high-level actions that the soldier exposes.

Answer 3

Encapsulate the functions to provide a consistent UI even if you later change the logic. Naming conventions are up to you, but I normally don't use "getFoo()", but just "foo()" as accessors and "setFoo()" as setters.

• return reference-to-const when you can (Effective C++ Item #3).
• Prefer consts, enums, and inlines to using hard coded numbers (Item #4)
• provide unique naming conventions for your private members to distinguish them from arguments
• Use unsigned values where they make sense to move errors to compile time
• When const values, like maximums, apply to an entire class. Make them static.
• If you plan to inherit, make sure your destructors are virtual
• initialize all members to sane defaults

This is how the classes look after that. CodePad

#include <iostream>
#include <string>
#include <stdint.h>

using namespace std;

class Gun 
{
public:
   Gun() : _bullets(0) {}
   virtual ~Gun() {}
   void fire() {cout << "bang bang" << endl; _bullets--;}
   void load(const uint16_t bullets) {_bullets = bullets;}
   const int bullets() const {return _bullets;}

   static const uint16_t MAX_BULLETS = 17;

protected:
   int _bullets;
 };

class Soldier 
{
public:
   Soldier(const string &name, const Gun &gun) : _name(name), _gun(gun) {}
   virtual ~Soldier() {}
   const string& name() const;
   Gun& gun() {return _gun;}

protected:
   string _name;
   Gun _gun;
};


int main (int argc, char const *argv[])
{
   Gun gun; // initialize
   string name("Foo");
   Soldier soldier(name, gun);

   soldier.gun().load(Gun::MAX_BULLETS);

   for(size_t i = 0; i < Gun::MAX_BULLETS; ++i)
   {
     soldier.gun().fire();
     cout << "I have " << soldier.gun().bullets() << " left!" << endl;
   }
  return 0;
}

Answer 4

Provide a "getGun()" or simply "gun()".

Imagine one day you may need to make that method more complex:

Gun* getGun() {
  if (!out_of_bullets_) {
    return &gun_;
  } else {
    PullPieceFromAnkle();
    return &secret_gun_;
  }
}

Also, you may want to provide a const accessor so people can use a const gun on a const soldier:

const Gun &getGun() const { return gun_; }

Answer 5

The Law of Demeter would say to encapsulate the functions.

http://en.wikipedia.org/wiki/Law_of_Demeter

This way, if you want some type of interaction between the soldier and the gun, you have a space to insert the code.

Edit: Found the relevant article from the Wikipedia link: http://www.ccs.neu.edu/research/demeter/demeter-method/LawOfDemeter/paper-boy/demeter.pdf The paperboy example is very, very similar to the soldier example you post.

Answer 6

First off, you'd be violating the Law of Demeter by accessing the Gun from outside the Soldier class.

I would consider methods like these instead:

soldier.ArmWeapon(...);
soldier.Attack(...);

This way you could also implement your fist, knife, grenade, baseball bat, laser cat, etc.