0
\$\begingroup\$

I have a medium sized project that uses a class hierarchy that essentially boils down to:

class Mach
{
 public:
    virtual ~Mach();
    //...
    virtual void common_action() =0;
    //... A lot of common actions
    //...
};

class MachStrato : public Mach
{
 public: //...
    // ...Implementation of the common interface
    virtual void common_action() override { /* ... */ }
    //...

    // ...But also some actions specific to this machine
    void specific_strato_action();
};

class MachFloat : public Mach
{
 public: //...
    // ...Implementation of the common interface
    virtual void common_action() override { /* ... */ }
    //...

    // ...But also some actions specific to this machine
    void specific_float_action();
};

The code uses just one instance of one these classes, instantiated in the early stages depending on external arguments. The code uses predominantly the common Mach interface, but sometimes have to probe the machine actual type to access its specific actions. In order to avoid repeating dynamic_cast in each of these cases but to do it once, I decided to wrap them in a class that, in addition to managing the memory, provides me with information on the actual type of the object; a sort of hierarchy-aware custom smart pointer:

// Create, take ownership and invoke the machine instance
class MachInstance
{
 public:
    MachInstance() noexcept :
        i_mach(nullptr),
        i_stratomach(nullptr),
        i_floatmach(nullptr) {}

    MachInstance(Mach* const m) noexcept :
        i_mach(m), 
        i_stratomach(dynamic_cast<MachStrato*>(m)), 
        i_floatmach(dynamic_cast<MachFloat*>(m)) {}

    ~MachInstance() { clear(); }

    // Take ownership of an existing machine (!contract: on the heap!)
    MachInstance& operator=(Mach* const m)
       {
        clear();
        i_mach = m;
        i_stratomach = dynamic_cast<MachStrato*>(i_mach);
        i_floatmach = dynamic_cast<MachFloat*>(i_mach);
        return *this;
       }

    void create( const std::string& arg )
       {
        clear();
        if( arg.find("float")!=std::string::npos )
           {
            i_mach = new MachFloat(arg);
           }
        else
           {
            i_mach = new MachStrato(arg);
           }
        i_stratomach = dynamic_cast<MachStrato*>(i_mach);
        i_floatmach = dynamic_cast<MachFloat*>(i_mach);
       }

    void clear()
       {
        if(i_mach)
           {
            delete i_mach;
            i_mach = nullptr;
           }
        i_stratomach = nullptr;
        i_floatmach = nullptr;
       }
 
    Mach* release()
       {
        Mach* m = i_mach;
        i_mach = nullptr;
        clear();
        return m;
       }

    operator bool() const { return i_mach!=nullptr; }
    Mach* ptr() const { return i_mach; }
    Mach* operator->() const { return i_mach; }
    
    // Unchecked
    MachStrato* as_strato() const { return i_stratomach; }
    MachFloat* as_float() const { return i_floatmach; }
    
private:
    Mach* i_mach;
    MachStrato* i_stratomach;
    MachFloat* i_floatmach;
    //...Other types may be -laboriously- added
    // Note: Do not use a union, a specialization could be nullptr
    //union  {
    //        Mach* i_mach;
    //        MachStrato* i_stratomach;
    //        MachFloat* i_floatmach;
    //       };

    MachInstance(const MachInstance&)=delete; // Prevent copy
    MachInstance& operator=(const MachInstance&)=delete; // Prevent copy assignment
};

The usage is like this:

MachInstance mach;
// Instantiate machine basing on external arguments
mach.create("strato");
// Accessing common interface
if(mach) mach->common_action();
// Accessing specific interface
if( user_wants_strato_action )
   {
    if( !mach.as_strato() ) throw std::runtime_error("Need a strato machine for this!");
    mach.as_strato()->specific_strato_action();
   }
if( user_wants_float_action )
   {
    if( !mach.as_float() ) throw std::runtime_error("Need a float machine for this!");
    mach.as_float()->specific_float_action();
   }

I quite like the usage of MachInstance, but what bothers me is that is not very scalable; if the machine types are n, it contains n+1 different pointers. I was wondering what you think of this strategy and if would you approach this problem differently.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ This seems like a theoretical exercise. Is this indeed theoretical or is it actually part of an existing project? Please take a look at the help center. \$\endgroup\$ – Mast May 12 at 19:53
  • \$\begingroup\$ @Mast I'm currently using MachInstance. This is the first time I'm posting here, so your comment is making me unsure if this question has sense in this space. Do you think it would be better to delete it? \$\endgroup\$ – MatG May 12 at 20:33
  • \$\begingroup\$ I do not know your program and libraries like you do, but considering it's answered you should probably leave it be. But it's definitely something to keep in mind for your next question. \$\endgroup\$ – Mast May 13 at 9:47
3
\$\begingroup\$

Design/code review

There is nothing to review in the first section… because there’s just no code there. The only comment to make is:

virtual void common_action() override { /* ... */ }

Use virtual OR override… not both. (Actually, the rule is “use only one of virtual, override, or final… but you should almost never use final.)

So the only thing to really review is the MachInstance class. But you’ve asked some specific questions, so what I’ll do is incorporate most of the review in the answers.

Here, I’ll just focus on some high-level stuff.

I don’t think the design of MachInstance makes any sense at all. I’m not referring to all the pointers in the class, I’m referring to the very idea of the class at all. You’ve basically rewritten std::unique_ptr<Mach>. Seriously. Other than the create() function… which I’ll get back to… this is just std::unique_ptr<Mach>. You could drop the whole class, and your code would look like this:

auto create(std::string const& arg) -> std::unique_ptr<Mach>
{
    if (arg.contains("float"))
        return std::unique_ptr<Mach>{new MachFloat(arg)};
    else
        return std::unique_ptr<Mach>{new MachStrato(arg)};
}

auto mach = create("strato");

// Accessing common interface
if(mach) mach->common_action();

// Accessing specific interface
if (user_wants_strato_action)
{
    if (auto const p = dynamic_cast<MachStrato*>(mach.get()); p)
        p->specific_strato_action();
    else
        throw std::runtime_error{"Need a strato machine for this!"};
}
if (user_wants_float_action)
{
    if (auto const p = dynamic_cast<MachFloat*>(mach.get()); p)
        p->specific_float_action();
    else
        throw std::runtime_error{"Need a float machine for this!"};
}

The interface is pretty much the same.

You’ll notice I pulled the create() function out and made it a free function. Even if you keep MachInstance, the create() function is a bad idea. That is two-phase initialization, and it has been frowned upon since at least the late-1990s.

The whole point of constructors is to… yanno… construct the object. Your constructor is leaving the object in a half-assed, unfinished state, which you then have to use create() to actually complete. That actually adds a ton of complexity to the class, because you have to contend with the possibility of the object being in the half-assed, unfinished state. If you just used the constructor to… yanno… construct the object… completely… then you don’t need much of the complexity of the class.

In any case, in a decent design, the only function you need to modify when adding a new machine type is create() (I’ll show how later). I would suggest either making it a free function, or, maybe, a static member function of Mach, so you’d write auto mach = Mach::create("strato"). (The logical place to put create(), in that case, is in an implementation file, so that when you do add a new machine type, you only have to recompile that one file, then relink.)

I think that’s about it for the overall design review. So, here are some suggestions for fixing your issue (with more review details sprinkled throughout).

Option 1: ditch inheritance and virtual functions

Back in the before-before times, C++ beginners were taught that The Way™ to do complicated things in C++ is to create a base class, derive concrete implementation classes from it, and use virtual functions to get polymorphic behaviour. Things have changed, and today, when I teach C++ to beginners, I teach that the using dynamic polymorphism should be a LAST resort, to be used only after you have considered every other possible design option, and rejected all of them.

So let me suggest a different way of approaching the problem.

  1. Don’t create a base class unless it actually helps. In other words, don’t create a base class just because you need a base class to do polymorphic dynamic dispatch. In your case, there’s apparently a ton of common behaviour… or is there? Not enough code to tell. If your virtual common_action() functions aren’t really common—as in, each class is different—then it makes no sense to put them in the base class. But if they all do have common behaviour, then sure, put that in the base.

  2. If you do create a base class, don’t bother making any virtual functions (including the destructor). That just causes a vtable to be generated, which is a waste of time.

  3. Create your classes for each “thing”: “machines” in your case. They don’t need a common base. They don’t even need a common interface. (Although, when a common interface makes sense, then sure, it makes sense.)

  4. Use a sum type to collect your classes. The standard sum type is std::variant.

  5. Use visitation to get polymorphic behaviour.

So here’s how that might look:

class MachStrato
{
public:
    constexpr void common_action();

    constexpr void specific_strato_action();

    // etc.
};

class MachFloat
{
public:
    constexpr void common_action();

    constexpr void specific_float_action();

    // etc.
};

class Mach
{
public:
    constexpr explicit Mach(std::string_view s) :
        _mach{_create(s)}
    {}

    // the shared interface
    constexpr void common_action()
    {
        std::visit([](auto&& m) { m.common_action(); }, _mach);

        // if a machine doesn't implement common_action(), you'll get a
        // compile-time error
    }

    // if you want a specific machine type, you can get it like this
    template <typename T> constexpr auto as()      &  -> T&        { return std::get<T>(_mach); }
    template <typename T> constexpr auto as() const&  -> T const&  { return std::get<T>(_mach); }
    template <typename T> constexpr auto as()      && -> T&&       { return std::get<T>(std::move(_mach)); }
    template <typename T> constexpr auto as() const&& -> T const&& { return std::get<T>(std::move(_mach)); }

    // to query the type
    template <typename T> constexpr auto is() const noexcept { return std::holds_alternative<T>(_mach); }

private:
    // there are two places you have to describe a new machine

    // #1
    using machine_types = std::variant<
        MachStrato,
        MachFloat,
        // ... add a new machine here
    >

    // #2
    static constexpr auto _create(std::string_view s)
    {
        // whatever the logic to determine which machine to create from a
        // string, put it here

        if (name.contains("float"))
            return MachFloat{s};
        else // the default?
            return MachStrato{s};
    }

    machine_types _mach;
};

// usage:
auto mach = Mach{"strato"};

mach.common_action();

// Accessing specific interface
if (user_wants_strato_action)
{
    mach.as<MachStrato>.specific_strato_action(); // will throw std::bad_variant_access if wrong type
}
if (user_wants_float_action)
{
    
    if (not mach.is<MachFloat>())
        throw std::runtime_error("Need a float machine for this!");
    mach.as<MachFloat>.specific_float_action();
}

Where possible, this can be optimized much better than the version with dynamic dispatch. And it can be constexpr.

Option 2: use templates (and smart pointers!)

So, for this this option, leave your Mach and its derived classes exactly as is. MachInstance is where all the magic will happen.

First, stop manually managing memory. It’s 2021. We use smart pointers now.

The trick here is to take all those as_strato(), as_float() functions, and use a single template instead.

class MachInstance
{
public:
    // is it really necessary to be able to create an "empty" machine?
    constexpr MachInstance() noexcept  = default;

    explicit MachInstance(std::unique_ptr<Mach> m) noexcept :
        _mach{std::move(m)}
    {}

    // no need to write destructor
    //
    // class is automatically non-copyable
    // class is automatically no-fail movable

    // Take ownership of an existing machine (note, *must* be on the heap, because smart pointer)
    auto operator=(std::unique_ptr<Mach> m) noexcept -> MachInstance&
    {
        _mach = std::move(m);
        return *this;
    }

    auto create(std::string const& arg)
    {
        // clear(); <- don't clear before constructing the new data; what if it fails?

        if (arg.contains("float"))
        {
            _mach.reset(new MachFloat(arg));
        }
        else
        {
            _mach.reset(new MachStrato(arg));
        }
    }

    // are these necessary? don't add stuff to the interface you don't need
    auto clear() noexcept { _mach.reset(); }
    auto release() noexcept { return _mach.release(); }
    explicit operator bool() const noexcept { return bool(_mach); }

    // in the standard pointer interface, this is spelled `get()`
    Mach* ptr() const { return i_mach; }

    auto operator->() const { return _mach; }

    // might as well add this
    // if you don't allow an "empty" state, then this could be noexcept
    auto operator*()       -> Mach      & { return *_mach; }
    auto operator*() const -> Mach const& { return *_mach; }

    template <typename T> constexpr auto as()       -> T*        { return dynamic_cast<T      *>(_mach.get()); }
    template <typename T> constexpr auto as() const -> T const*  { return dynamic_cast<T const*>(_mach.get()); }

private:
    std::unique_ptr<Mach> _mach;
};

Note: that’s the entire MachInstance class. Nothing’s missing (that I can think of), and in fact, I think many more of those functions can go. Less is more. The more crap you put in a class, the more chance of stuff breaking. (And of course, the more you have to test, etc..)

And the usage:

MachInstance mach;
// Instantiate machine basing on external arguments
mach.create("strato");
// Accessing common interface
if(mach) mach->common_action();
// Accessing specific interface
if( user_wants_strato_action )
   {
    if( !mach.as<MachStrato>() ) throw std::runtime_error("Need a strato machine for this!");
    mach.as<MachStrato>()->specific_strato_action();
   }
if( user_wants_float_action )
   {
    if( !mach.as<MachFloat>() ) throw std::runtime_error("Need a float machine for this!");
    mach.as<MachFloat>()->specific_float_action();
   }

Note that the only thing I changed in this interface is the as_strato() and as_float() function calls. Everything else is untouched.

\$\endgroup\$
2
  • \$\begingroup\$ I'll definitely start new branches and try some of your interesting ideas. I think they shed some needed light in this old codebase. Any reason why you didn't use std::make_unique? \$\endgroup\$ – MatG May 13 at 6:24
  • \$\begingroup\$ Yeah, make_unique() is obsolete as of C++17. It’s not deprecated or anything, so you can still use it if you feel like it. It used to have a purpose for safety due to unspecified order of operations if you did f(unique_ptr<int>{new int}, unique_ptr<double>{new_double}), but that was fixed, so it serves no purpose anymore. I think using unique_ptr’s constructor directly is clearer, and more flexible, too, because you can do unique_ptr{new T} or unique_ptr{new T{}}… rather than make_unique<T>() versus make_unique_for_overwrite<T>(). \$\endgroup\$ – indi May 13 at 10: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.