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

There's no golden rule that applies 100% of the time. It's really a judgement call depending on your needs.

It depends on how much functionality you want to hide/disallow for the gun from access to the Solider.

If you want to have only read only access to the Gun you could return a const reference to your own member.

If you want to expose only certain functionality you could make wrapper functions. If you don't want the user to try to change Gun settings through the Soldier then make wrapper functions.

Generally though, I see the Gun as it's own object and if you don't mind exposing all of Gun's functionality, and don't mind allow things to be changed through the Soldier object, just make it public.

You probably don't want a copy the gun so if you make a GetGun() method make sure that you aren't returning a copy of the gun.

If you want to keep your code simple then have the soldier responsible for dealing with the gun. Does your other code need to work with the gun directly? Or can a soldier always know how to work/reload his own gun?

Answer 2

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 3

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 4

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 5

Indeed, it depends a lot about how much control you want to have.

To model the real world, you might even want to completely encapsulate the gun object, and just have a soldier.attack() method. The soldier.attack() method would then see whether the soldier was carrying a gun, and what the state of the gun was, and fire or reload it as necessary. Or possibly throw the gun at the target and run away, if insufficient ammunition were present for either operation...

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.

Answer 7

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 8

If you expose gun, you allow things beyond the member functions of the Gun, which is probably not a good idea:

soldier.gun = anotherGun; // where did you drop your old gun?

If you use getGun(), the calls look a little ugly, but you can add functions to Gun without modifying Soldier.

If you encapsulate the functions (which I recommend) you can modify the Gun or introduce other (derived) classes of Gun without changing the interface to Soldier.

Answer 9

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;
}