Best practice for factory pattern in c++?

Question

I'm implementing a simple data reader to get image sequences from either a pre-recorded video file (In my case ROS bag file), or a camera device (In my case Intel Realsense). Basically I intend for users to use it in the following way:

1. When a non-empty file path is provided, the reader reads from the video file; otherwise if an empty path is specified, the reader reads from the camera device. (For now in case of any failure, throwing exceptions and terminating the program will be fine.)

2. The user iteratively calls bool get_next_frame(Frame&) to get all the frames, until reaching the end of the video, upon which the function returns false.

I'm trying to implement the above-mentioned functionality using the factory pattern, in C++ as follows:

class DataReader;
typedef std::shared_ptr<DataReader> datareader_ptr;

class DataReader {
  friend datareader_ptr make_data_reader(const std::string& ros_bag_path);
public:
  virtual bool get_next_frameset(rs2::frameset& frames)=0;
  virtual void finalize() {
    pipe_.stop();
  }
  virtual ~DataReader() {
    std::cout << "dtor for DataReader called." << std::endl;
    finalize();
  }
protected:
  DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
      :cfg_(cfg),pipe_(pipe),pipeline_profile_(pipeline_profile) {}
  rs2::config cfg_;
  rs2::pipeline_profile pipeline_profile_;
  rs2::pipeline pipe_;
};

class RosBagDataReader : public DataReader {
  friend datareader_ptr make_data_reader(const std::string& ros_bag_path);
public:
  virtual bool get_next_frameset(rs2::frameset& frames) {
    frames = pipe_.wait_for_frames();
    return true;
  }
  virtual ~RosBagDataReader() {}
protected:
  RosBagDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
      :DataReader(cfg, pipe, pipeline_profile) {}
};

class CameraDataReader : public DataReader {
  friend datareader_ptr make_data_reader(const std::string& ros_bag_path);
public:
  virtual bool get_next_frameset(rs2::frameset& frames) {
    frames = pipe_.wait_for_frames();
    return true;
  }
  virtual ~CameraDataReader() {}
protected:
  CameraDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
      :DataReader(cfg, pipe, pipeline_profile) {}
};

datareader_ptr make_data_reader(const std::string& ros_bag_path) {
  rs2::config cfg;
  rs2::pipeline pipe;
  if (ros_bag_path=="") {
    rs2::pipeline_profile profile = pipe.start();
    return datareader_ptr(new CameraDataReader(cfg, pipe, profile));
  } else {
    // from ros bag file
    cfg.enable_device_from_file(ros_bag_path, false);
    rs2::pipeline_profile profile = pipe.start(cfg); //File will be opened in read mode at this point
    rs2::playback play(profile.get_device());
    play.set_real_time(false);
    return datareader_ptr(new RosBagDataReader(cfg, pipe, profile));
  }
}

However, I'm really not sure whether this is the best practice. More specifically,

1. Is the usage of factory pattern here justified ?
2. Is my implementation of factory pattern correct ? Or are there any pitfalls in the code ?

For example, I used friend function to access the protected constructors in all the classes, is this appropriate or there are better ways to do this?

Another example, protected constructor is used so that the users may not directly construct the readers, but only through the factory function. Is this done correctly?

Thanks in advance!

Update: Definitions of stuff in namespace rs2: may refer to this doc. Basically they are all just thin wrappers holding a shared_ptr to the class doing the real work. (i.e. pimpl idiom)

Answer 1

I’ll start by answering the questions.

Questions

Is the usage of factory pattern here justified ?

🤷🏼

Going just by what’s visible in the code and your commentary, I don’t see any purpose to writing the code this way. I don’t see any purpose to the RosBagDataReader or CameraDataReader classes. They don’t actually do anything unique; you could just implement get_next_frameset() in the base class, and forget the whole hierarchy.

In fact, the only difference at all between the classes is not what they do, but rather how their data is set up. I suppose you could sorta-kinda justify having different classes to handle the different types of initialization… but then, you should put the initialization in the classes—that is, in the constructors. However, you could just as easily use two constructors for DataReader, and again, forget the rest of the hierarchy.

But the thing that really makes me scratch my head about why you’d want to use the factory pattern is the fact that you don’t even really have a single factory. You have two factories that you’ve just shoehorned into one by saying “go one way if there’s a path, the other way if not”. That shouldn’t be a single function; that’s poor design. That should be two functions, one taking a path, one not. And once you’ve done it that way, it kinda reveals that the factory pattern doesn’t really make sense here.

Frankly, run-time polymorphism should always be the last tool you reach for in modern C++:

• It relies on reference semantics, and C++ is a value semantic language.
• It usually means you need to use dynamic allocation, which is slow, cumbersome, and error-prone (though smart pointers help).
• It doesn’t really work well with compile-time stuff like type-checking and constexpr… which, again, are C++’s strengths.
• It’s often less efficient because of the extra memory used, and extra indirection.
• And there are all kinds of nasty gotchas just waiting to bite your ass (like, slicing if you aren’t VERY careful writing copy ops, swap ops, and so on).

HOWEVER….

Of course, there are situations where the factory pattern using run-time polymorphism is the perfect solution. One that springs to mind is the case where you’re writing a library: you’d only give DataReader and the factory function as the entire library interface, and client code can use the library without knowing any details about possible concrete readers, so you could swap them out without having to recompile the client. You could even add readers as plugins.

If you were doing that kind of thing, then sure, the factory pattern makes sense. But… if you’re just writing a program—not a library, let alone an extensible library—then I don’t see the point of it.

Is my implementation of factory pattern correct ? Or are there any pitfalls in the code ?

Eh, it’s mostly okay. It’s not outright wrong, but… there are a few problems with doing things the way you’re doing them.

The first issue is that you don’t seem to know whether you’re writing an interface hierarchy, or an implementation hierarchy. Those are two very different things, and they need to be handled in different ways. If you are trying to build from an interface—in this case, the single function get_next_frameset()—then your base class should be a pure abstract class… what other languages like Java specifically call an interface: no data, only pure virtual functions. If you are building up an implementation, then you’d do things very differently.

I can’t tell you which one you’re really doing here, because, frankly, you don’t have much of an interface or implementation hierarchy, and none of the commentary explains any of the reasoning for even wanting the factory pattern in the first place. As I noted earlier, there doesn’t really seem to be much justification for a hierarchy at all.

I’m also not keen on using shared pointers as the default. It seems completely pointless, especially because you can’t even take advantage of std::make_shared() (because the constructors aren’t public). You should probably return a std::unique_ptr instead. If someone wants a shared pointer, they can always make one out of the unique pointer… but you can’t really do the opposite.

For example, I used friend function to access the protected constructors in all the classes, is this appropriate or there are better ways to do this?

I actually think pretty much all of your uses of protected are bad. But let’s set that aside for now.

If the constructors are non-public, then yeah, you have to use a friend somehow. There’s really no choice.

Another example, protected constructor is used so that the users may not directly construct the readers, but only through the factory function. Is this done correctly?

I honestly don’t see the sense of making any of the constructors protected.

Let’s start with the base class constructor. The only reason you need it protected is because you want initialize the protected data members. Now, I think the protected data members are also a bad idea, but one thing at a time. If you are going to have data members in the base class—which, again, is generally unwise—then, okay, it makes sense to have a protected constructor.

As for the constructors of the other two classes, unlike the base class these are concrete classes. What is the reasoning for hiding those constructors? If you want people to use the factory function, fine, but why make that the only option? Why don’t you want users to directly construct the readers?

By hiding those constructors, you prevent the use of std::make_shared(), which is an optimization opportunity lost. What’s the logic of that?

Also, even if you wanted to hide those constructors, what’s the sense in making them protected, and not private? That seems to imply you expect the classes to be derived from… but that’s a terrible idea: non-leaf classes should be abstract. (Again, assuming you’re doing interface inheritance. If you’re doing implementation inheritance, then there’s a whole other set of rules.)

Code review

Because the code is not presented as it would be in the actual project, and is instead all packed into a single block as some kind of code soup, with no includes and no explanation of what some of the key types are, this isn’t going to be a very comprehensive code review. I’ll do what I can with what I’ve got, but as the saying goes, GIGO.

typedef std::shared_ptr<DataReader> datareader_ptr;

There’s no indication of why shared_ptr is used here, and not unique_ptr, and in fact the rest of the code is written in a way that hamstrings the use of shared_ptr.

virtual void finalize() {
  pipe_.stop();
}
virtual ~DataReader() {
  std::cout << "dtor for DataReader called." << std::endl;
  finalize();
}

NEVER call virtual functions from constructors or destructors. It doesn’t do what you’d think, and only creates confusion for less experienced programmers.

In this case it “works” only because… well, I mean, the whole hierarchy is actually pointless. If the hierarchy were actually doing something that justified its existence, then this would almost certainly not work.

There is no visible purpose to finalize(). Just put pipe_.stop() in the destructor.

DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
    :cfg_(cfg),pipe_(pipe),pipeline_profile_(pipeline_profile) {}

This is not a good way to write this constructor. (Or maybe it is? Who knows? Not enough information, forced to make guesses.)

If you are going to take the arguments by value—which is the right thing to do, because they are sink arguments—then you should move them into their final place… not copy. The copy is unnecessary, and possibly wasteful. And, if the move ops for all those types is noexcept (which it should always be), then that would mean you could make the constructor noexcept too.

The other issue is that you’ve mixed up the order of construction; it’s not the same as the order of declaration:

// order of declaration:
rs2::config           cfg_;
rs2::pipeline_profile pipeline_profile_;
rs2::pipeline         pipe_;

// order of construction:
cfg_(cfg),
pipe_(pipe),
pipeline_profile_(pipeline_profile)

I’m surprised this compiled without warning. You do have all warnings turned on, don’t you?

By the way, all the above is true of all the constructors, because they’re all identical. Which… again… is another issue.

class RosBagDataReader : public DataReader {
  // ... [snip] ...

  virtual bool get_next_frameset(rs2::frameset& frames) {

Prefer one of virtual, override, final, or none of the above. In this case, the one you want is NOT virtual, it’s override.

virtual ~RosBagDataReader() {}

You don’t need this. Don’t write code you don’t need; it just becomes a maintenance burden, noise obscuring important code, and confusion for future coders who wonder why it was put there if it wasn’t necessary.

// in base class:
virtual bool get_next_frameset(rs2::frameset& frames)=0;
rs2::pipeline pipe_;

// in derived class 1:
bool get_next_frameset(rs2::frameset& frames) override {
  frames = pipe_.wait_for_frames();
  return true;
}

// in derived class 2
bool get_next_frameset(rs2::frameset& frames) override {
  frames = pipe_.wait_for_frames();
  return true;
}

Why?

Why not:

// in base class:
virtual bool get_next_frameset(rs2::frameset& frames) {
  frames = pipe_.wait_for_frames();
  return true;
}
rs2::pipeline pipe_;

// in derived class 1:
// nothing needed

// in derived class 2:
// nothing needed

This entire class hierarchy serves no purpose. If I were to reorganize it into something logical, it might look more like this:

// truly abstract base class
class DataReader
{
public:
    virtual ~DataReader() /* = default; */
    {
        std::cout << "dtor for DataReader called." << std::endl;
    }

    virtual bool get_next_frameset(rs2::frameset& frames) = 0;
};

// concrete implementation 1
class RosBagDataReader : public DataReader
{
    // all data is private, as it should be
    rs2::config cfg_;
    rs2::pipeline_profile pipeline_profile_;
    rs2::pipeline pipe_;

public:
    RosBagDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipeline_profile_(std::move(pipeline_profile)),
        pipe_(std::move(pipe))
    {}

    ~RosBagDataReader()
    {
        pipe_.stop();
    }

    bool get_next_frameset(rs2::frameset& frames) override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }
};

// concrete implementation 2... same as 1... which, yeah
class CameraDataReader : public DataReader
{
    rs2::config cfg_;
    rs2::pipeline_profile pipeline_profile_;
    rs2::pipeline pipe_;

public:
    CameraDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipeline_profile_(std::move(pipeline_profile)),
        pipe_(std::move(pipe))
    {}

    ~CameraDataReader()
    {
        pipe_.stop();
    }

    bool get_next_frameset(rs2::frameset& frames) override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }
};

Or:

// no longer abstract, or even designed for inheritance at all:
class DataReader
{
    // all data is private, as it should be
    rs2::config cfg_;
    rs2::pipeline_profile pipeline_profile_;
    rs2::pipeline pipe_;

public:
    DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipeline_profile_(std::move(pipeline_profile)),
        pipe_(std::move(pipe))
    {}

    ~DataReader()
    {
        std::cout << "dtor for DataReader called." << std::endl;
        pipe_.stop();
    }

    bool get_next_frameset(rs2::frameset& frames)
    {
        frames = pipe_.wait_for_frames();
        return true;
    }
};

// now pointless: class RosBagDataReader
// now pointless: class CameraDataReader

Honestly, I don’t even know if the suggestions I’m making make any sense whatsoever, because without the definitions of rs2::config et cetera, I have no clue whether these classes are copyable, movable, or whatever. But I don’t know what else to say: you hide code from reviewers, you get shitty reviews.

datareader_ptr make_data_reader(const std::string& ros_bag_path)

My biggest peeve with this factory function is that the argument list is bullshit. If I want to construct a data reader for a camera, I have to create a completely spurious path. That’s a waste of time for me on every level: it’s a waste of time for me to write the code to create a path I neither want nor need, and it’s a waste of resources within the program to create this useless path only to make a pointless check that it’s empty and then discard it.

If I were actually going to use this code, the very first thing I’d do is write my own wrapper functions:

auto make_camera_reader() { return make_data_reader({}); }

auto make_ros_bag_reader(std::string const& path) { return make_data_reader(path); }

And then I’d use those instead of direct calls to the factory functions.

Why? Because if I saw this in code:

auto reader = make_data_reader({}); // or make_data_reader("") or whatever

I’d immediately be forced to wonder… what is the {}? What does it mean? What am I constructing there at the call site? Will it be expensive? Does it have some semantics I should be aware of?

I like reading mystery stories; I don’t like mystery code. Code shouldn’t challenge me to wonder what’s going on, or force me to go read the docs. It should be crystal clear about what’s going on, and what it’s doing.

On the other hand, if I were to see:

auto reader = make_camera_reader();

That tells me pretty much everything I need to know. No mysteries.

This is not a problem with factory functions in general. Factory functions can still be very clear about what they’re doing. For example:

auto renderer = make_renderer("opengl"); // or make_renderer("vulkan") or whatever

// Here, make_renderer returns unique_ptr<renderer>, and renderer is an
// abstract base class, and the factory function returns concrete renderer
// implementations... but none of that matters! Just from the call above, I
// understand generally what's going on.

Compare that to:

auto reader = make_data_reader("");

// or even:
auto reader = make_data_reader("/path/to/file");

What is really going on there? “Data reader”? Well that’s damn vague. In the first case: the hell is that string supposed to mean? Make a data reader for… nothing? Emptiness? Perhaps the {} refers to wanting only default options? In the second case: okay, we’re making a data reader for some path, probably a file. That’s still pretty damned vague. Are we making a data reader for the file? If so, what kind of data? Are we making a data reader from the file?

If I were going to improve the design of your factory function, I might refactor it as follows:

auto make_data_reader(std::string_view name, std::string_view args)
{
    rs2::config cfg;
    rs2::pipeline pipe;

    if (name == "camera")
    {
        // args is unused

        rs2::pipeline_profile profile = pipe.start();
        return datareader_ptr(new CameraDataReader(cfg, pipe, profile));
    }
    else if (name == "video file")
    {
        // args is the path to the video file

        cfg.enable_device_from_file(args, false);

        rs2::pipeline_profile profile = pipe.start(cfg);

        rs2::playback play(profile.get_device());
        play.set_real_time(false);

        return datareader_ptr(new RosBagDataReader(cfg, pipe, profile));
    }
    /* future expansion:
        else if (name == "url")
        {
            // args is the url

            // ... implementation...
        }
    */
    else
    {
        throw std::logic_error{std::string{"unknown data reader type: "} + name};
    }

    // this should never happen, so it could also be an assert
    throw std::logic_error{std::string{"data reader type "} + std::string{name} + " failed to return reader"};
}

inline auto make_data_reader(std::string_view name)
{
    return make_data_reader(name, {});
}

And the usage is:

// I want to read frames from a camera:
auto reader = make_data_reader("camera");

// I want to read frames from a file:
auto reader = make_data_reader("video file", "/path/to/file");

// Hypothetical extension: I want to read frames from a web service:
auto reader = make_data_reader("url", "https://domain.com/path/to/service");

Let’s keep following the train of thought I started with my wrapper functions. What if they were more than mere wrappers?

auto make_camera_reader()
{
    rs2::config cfg;
    rs2::pipeline pipe;
    rs2::pipeline_profile profile = pipe.start();

    // Note: using make_shared because I assume constructors are public.
    return std::make_shared<CameraDataReader>(std::move(cfg), std::move(pipe), std::move(profile));
}

// Note: taking a proper path rather than a random string. You should always
// prefer proper, semantic types.
auto make_ros_bag_reader(std::filesystem::path const& path)
{
    rs2::config cfg;
    cfg.enable_device_from_file(ros_bag_path.string(), false);

    rs2::pipeline pipe;

    rs2::pipeline_profile profile = pipe.start(cfg);

    rs2::playback play(profile.get_device());
    play.set_real_time(false);

    return std::make_shared<RosBagDataReader>(std::move(cfg), std::move(pipe), std::move(profile));
}

Now there doesn’t seem to be any point to returning pointers, rather than objects directly.

In fact, let’s go one step further:

class image_sequence_reader // a better name than DataReader
{
    rs2::config cfg_;
    rs2::pipeline pipe_;
    rs2::pipeline_profile pipeline_profile_;

public:
    static auto make_for_camera() -> image_sequence_reader
    {
        rs2::config cfg;
        rs2::pipeline pipe;
        rs2::pipeline_profile profile = pipe.start();

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    static auto make_for_video_file(std::filesystem::path const& path) -> image_sequence_reader
    {
        rs2::config cfg;
        cfg.enable_device_from_file(ros_bag_path.string(), false);

        rs2::pipeline pipe;

        rs2::pipeline_profile profile = pipe.start(cfg);

        rs2::playback play(profile.get_device());
        play.set_real_time(false);

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    ~DataReader()
    {
        pipe_.stop();
    }

    auto get_next_frameset(rs2::frameset& frames) -> bool
    {
        frames = pipe_.wait_for_frames();
        return true;
    }

private:
    DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipe_(std::move(pipe)),
        pipeline_profile_(std::move(pipeline_profile))
    {}
};

Usage:

// I want to read frames from a camera:
auto reader = image_sequence_reader::make_for_camera();

// I want to read frames from a file:
auto reader = image_sequence_reader::make_for_video_file("/path/to/file");

Want to extend the capabilities to read from some other source? No problem. Just add a new static member function:

    static auto make_for_nondefault_camera(camera_id const& id) -> image_sequence_reader;
    static auto make_for_device(std::filesystem::path const& device_path) -> image_sequence_reader;
    static auto make_for_web_service(url const& location) -> image_sequence_reader;
    static auto make_for_retina_brain_interface(person const& name) -> image_sequence_reader;
    // and so on...

That’s much less work than adding a whole new derived class, and then monkeying with the factory function to be able to select it.

It’s also clearer—both in the class and at the call site—safer, and, frankly, just easier to use.

Okay, but what if you really, REALLY want to use the factory pattern, because this is going to be in a library, and you want to be able to extend the library without client code having to be recompiled.

With some minor tweaks, you can even still enjoy all the other benefits of the factory pattern too:

////////////////////////////////////////////////////////////////////////
// in header used by client code:

// pure interface:
class image_sequence_reader_interface
{
public:
    virtual ~image_sequence_reader_interface() = default;

    virtual auto get_next_frameset(rs2::frameset&) -> bool = 0;

    // run-time polymorphic classes should generally not be copyable.
    // usually you'd implement a clone() function. i'll skip all that here.
};

// concrete implementation: the only thing client code really needs to care
// about.
class image_sequence_reader final : public image_sequence_reader_interface
{
    // if necessary, you can use a custom deleter to make sure the library
    // deletes its own memory, and not the application.
    std::unique_ptr<image_sequence_reader_interface> _p_impl;

public:
    static auto make_for(std::string_view name, std::string_view args) -> image_sequence_reader;

    static auto make_for(std::string_view name) -> image_sequence_reader
    {
        return make_for(name, {});
    }

    ~image_sequence_reader() = default;

    auto get_next_frameset(rs2::frameset& frames) -> bool override
    {
        return _p_impl->get_next_frames(frames);
    }

    image_sequence_reader(image_sequence_reader&&) noexcept = default;
    auto operator=(image_sequence_reader&&) noexcept -> image_sequence_reader& = default;

private:
    explicit image_sequence_reader(std::unique_ptr<impl>) noexcept;
};

////////////////////////////////////////////////////////////////////////
// in implementation file (cpp file, or module, or library source code, depending):

class camera_image_sequence_reader : public image_sequence_reader_interface
{
    rs2::config cfg_;
    rs2::pipeline pipe_;
    rs2::pipeline_profile pipeline_profile_;

public:
    camera_image_sequence_reader() :
        camera_image_sequence_reader(_construct())
    {}

    ~camera_image_sequence_reader()
    {
        pipe_.stop();
    }

    auto get_next_frameset(rs2::frameset& frames) -> bool override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }

    camera_image_sequence_reader(camera_image_sequence_reader&&) noexcept = default;
    auto operator=(camera_image_sequence_reader&&) noexcept -> camera_image_sequence_reader& = default;

private:
    // All this _construct() stuff *MIGHT BE* needed because the data
    // members all seem to need to be constructed in a specific order, and
    // with references to each other. Or none of it might be necessary;
    // there's no way to know because I don't know what the data types are.

    static auto _construct() -> std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile>
    {
        rs2::config cfg;
        rs2::pipeline pipe;
        rs2::pipeline_profile profile = pipe.start();

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    camera_image_sequence_reader(std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile> t) :
        cfg_(std::move(t.cfg)),
        pipe_(std::move(t.pipe)),
        pipeline_profile_(std::move(t.profile))
    {}
};

class video_file_image_sequence_reader : public image_sequence_reader_interface
{
    rs2::config cfg_;
    rs2::pipeline pipe_;
    rs2::pipeline_profile pipeline_profile_;

public:
    video_file_image_sequence_reader() :
        video_file_image_sequence_reader(_construct())
    {}

    ~video_file_image_sequence_reader()
    {
        pipe_.stop();
    }

    auto get_next_frameset(rs2::frameset& frames) -> bool override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }

    video_file_image_sequence_reader(video_file_image_sequence_reader&&) noexcept = default;
    auto operator=(video_file_image_sequence_reader&&) noexcept -> video_file_image_sequence_reader& = default;

private:
    static auto _construct() -> std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile>
    {
        rs2::config cfg;
        cfg.enable_device_from_file(ros_bag_path.string(), false);

        rs2::pipeline pipe;

        rs2::pipeline_profile profile = pipe.start(cfg);

        rs2::playback play(profile.get_device());
        play.set_real_time(false);

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    video_file_image_sequence_reader(std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile> t) :
        cfg_(std::move(t.cfg)),
        pipe_(std::move(t.pipe)),
        pipeline_profile_(std::move(t.profile))
    {}
};

// add or include any other implementations

auto image_sequence_reader::make_for(std::string_view name, std::string_view args) -> image_sequence_reader
{
    auto p = std::unique_ptr<impl>{};

    if (name == "camera")
    {
        p.reset(new camera_image_sequence_reader{});
    }
    else if (name == "video file")
    {
        p.reset(new video_file_image_sequence_reader{args});
    }

    // add any other options ...

    else
    {
        throw std::logic_error{std::string{"unknown image sequence reader type: "} + name};
    }

    // assert(p);

    return image_sequence_reader{std::move(p)};
}

And the usage is the same as before:

// I want to read frames from a camera:
auto reader = image_sequence_reader::make_for("camera");

// I want to read frames from a file:
auto reader = image_sequence_reader::make_for("video file", "/path/to/file");

And not only that, but with very little extra work you could even allow clients to register their own extension implementations. Alls you need is a register function in image_sequence_reader that takes a name and a create function (which returns a unique pointer with an appropriate deleter), and then a map that maps names to create functions, and then in image_sequence_reader::make_for(), just query the map for the name, then use the create function.

Is this a better design than what you have? 🤷🏼 Impossible to say with the information I have.