Should all objects in a class be a reference?

Question

Is it bad practice to always make my object member's data type a reference?

After reading this question about avoiding #include I've have been trying to avoid using #include in my current C++ project and always forward declare my data types.

Here is an example code snippet from my project. In the header I have EnemyBuilder &m_builder; and then I initialize it in the source.

EnemyFactory.h

#include <string>

#ifndef ENEMYFACTORY
#define ENEMYFACTORY

class World;
class Enemy;
class EnemyBuilder;

class EnemyFactory
{
private:
    EnemyBuilder &m_builder;
    World &m_world;

public:
    EnemyFactory(World &world);

    Enemy* produce(std::string type);
};

#endif

EnemyFactory.cpp

#include "EnemyFactory.h"

#include "EnemyBuilder.h"
#include "World.h"
#include "Enemy.h"

EnemyFactory::EnemyFactory(World &world) :
    m_world(world), m_builder(EnemyBuilder())
{

}

Enemy* EnemyFactory::produce(std::string type)
{
    Enemy *product = new Enemy(m_world);

    if (type == "basic")
    {
        product->setSpeed(5000.0f);
    }

    return product;
}

Answer 1

Is it bad practice to always make my object member's data type a reference?

Not only is it bad practice, it's inefficient and restrictive.

Why?

1. Objects of classes that contain references are automatically non-assignable. The standard has std::reference_wrapper<> (basically a wrapped pointer) to get around this restriction when binding reference types to function objects.

2. You are obliged to manage object lifetimes. Since you're storing references rather than objects, your class has no possibility to clean up after itself. In addition, it can't ensure that the objects to which it is referring are still there. You've just lost all the protection that c++ gives you. Welcome to the stone age.

3. It's less efficient. All object access now gets dereferenced through what is essentially a pointer. And you will most-likely lose cache locality. Pointless.

4. you lose the ability to treat objects as values. This will results in less-efficient, convoluted programs.

5. I could go on...

What should I do?

Prefer value-semantics (treating objects as values). Let them be copied (most of the copies will be elided anyway if you write your classes nicely). Encapsulate and compose them into larger objects. Let the container manage the lifetime of the components. That's its job.

What about the time cost of including a few header files?

Unless you're doing hideously complex template expansion across hundreds (thousands) of template classes, it's unlikely to even approach being a problem. You waste more time making the coffee than you will compiling a few source files.

Answer 2

See Richard Hodges' answer first... In your current code, you are piling up kegs of gunpowder for yourself, you may just ignite it someday.... ...someday soon... To remedy that.

In EnemyFactory.h, you have a line... Prefer to use std::unique_ptr

 Enemy* produce(std::string type);

So that, this becomes

std::unique_ptr<Enemy> produce(std::string type);

Again in EnemyFactory.cpp....

You have the dreaded Undefined Behaviour, basically, your program will crash because you binded a reference to a temporary object

EnemyFactory::EnemyFactory(World &world) :
    m_world(world), m_builder(EnemyBuilder())  //<---This will crash
{

}

The bigger scope of what you are trying to do applies to the PIMPL idiom.

The best way to approach that will be

EnemyFactory.h

#include <string>
#include <memory>

#ifndef ENEMYFACTORY
#define ENEMYFACTORY

class World;
class Enemy;
class EnemyBuilder;

class EnemyFactory
{
private:
    std::unique_ptr<EnemyBuilder> m_builder;
    std::shared_ptr<World> m_world;  //<- I am guessing World is a shared object based on your constructor

public:
    EnemyFactory(World &world);

    std::unique_ptr<Enemy> produce(std::string type);
};

#endif

At EnempyFactory.cpp

#include "EnemyFactory.h"

#include "EnemyBuilder.h"
#include "World.h"
#include "Enemy.h"

//I hope it is guaranteed the instance if World, world will always outlive any instance of EnemyFactory...

EnemyFactory::EnemyFactory(World &world) :
    m_world(&world), m_builder(std::make_unique<EnemyBuilder>())
{

}

std::unique_ptr<Enemy> EnemyFactory::produce(std::string type)
{
    std::unique_ptr<Enemy> product = std::make_unique<Enemy>(m_world);

    if (type == "basic")
    {
        product->setSpeed(5000.0f);
    }

    return product;
}

We aren't using reference here because, all class members that are references must be instantiated in the constructor and managing lifetimes could be a mess, depends on your use case though....

Pointers are more handy here.... And remember, whenever pointers comes to your mind, prefer std::unique_ptrs first, when the object is shared then, you can go for 'std::shared_ptr`.

Answer 3

I think you are killing ants with a hammer. forward declaration is a tool, but not always is a "best practice". You must think in someone (or you) reading your code in future, they must understand what is your code doing.

I suggest you this approach:

EnemyFactory.h

 #include <string>

#ifndef ENEMYFACTORY
#define ENEMYFACTORY

class EnemyFactory
{
private:
    EnemyBuilder m_builder;
    World m_world;

public:
   EnemyFactory(World &world);

   Enemy* produce(std::string type);
};

#endif

EnemyFactory.cpp

#include "EnemyBuilder.h"
#include "World.h"
#include "Enemy.h"

#include "EnemyFactory.h"// this include must be last one
EnemyFactory::EnemyFactory(World &world)
{

}

Enemy* EnemyFactory::produce(std::string type)
{
    Enemy *product = new Enemy(m_world);

    if (type == "basic")
    {
        product->setSpeed(5000.0f);
    }

    return product;
}

A little change in include order and you could ommit forward declaration and references