CODEWITHSUNDEEP
c++c++11creation-pattern
John Dibling
John Dibling

Reputation: 101446

make_unique, factory method or different design for client API?

We have a library with that publishes an abstract base class:

(illustrative psudocode)

/include/reader_api.hpp

class ReaderApi
{
public:
  static std::unique_ptr <ReaderApi> CreatePcapReader ();
  virtual ~ReaderApi() = 0;
};

In the library implementation, there is a concrete implementation of ReaderApi that reads pcap files:

/lib/pcap_reader_api.cpp

class PcapReaderApi
: 
  public ReaderApi
{
public:
  ReaderApi() {};
};

Client code is expected to instantiate one of these PcapReaderApi objects via the factory method:

std::unique_ptr <ReaderApi> reader = ReaderApi::CreatePcapReader ();

This feels gross to me on a couple of levels.

First, the factory method should be free, not a static member of ReaderApi. It was made a static member of ReaderApi to be explicit about the namespaces. I can see pros & cons either way. Feel free to comment on this, but it's not my main point of contention.

Second, my instinct tells me I should be using std::make_unique rather than calling a factory method at all. But since the actual object being made is an implementation detail, not part of the public headers, there's nothing for the client to make_unique.

The simplest solution, in terms of understandability and code maintenance, appears to be the solution I've already come up with above, unless there is a better solution that I'm not already aware of. Performance is a not a major consideration here since, because of the nature of this object, it will only be instantiate once, at startup-time.

With code clarity, understandability, and maintainability in mind, is there a better way to design the creation of these objects than I have here?

I have considered two alternatives that I'll go over below.

One alternative I've considered is passing in some kind of identifier to a generic Create function. The identifier would specify the kind of object the client wishes to construct. It would likely be an enum class, along these lines:

enum class DeviceType
{ 
  PcapReader
};

std::unique_ptr <ReaderApi> CreateReaderDevice (DeviceType);

But I'm not sure I see the point of doing this versus just making the create function free and explicit:

std::unique_ptr <ReaderApi> CreatePcapReader ();

I also thought about specifying the DeviceType parameter in ReaderApi's constructor:

class ReaderApi
{
public:
  ReaderApi (DeviceType type);
  virtual ~ReaderApi() = 0;
};

This would enable the make_unique idiom:

std::unique_ptr <ReaderApi> reader = std::make_unique <ReaderApi> (DeviceType::PcapReader);

But this obviously would present a big problem -- you're actually trying to construct a ReaderApi, not a PcapReader. The obvious solution to this problem is to implement a virtual constructor idiom or use factory construction. But virtual construction seems over-engineered to me for this use.

Upvotes: 1

Views: 1249

Answers (1)

Mark B
Mark B

Reputation: 96233

To me, the two options to consider are your current approach, or a namespace level appropriately named free function. There doesn't seem to be a need for an enumerated factory unless there are details you aren't mentioned.

Using make_unique is exposing implementation details so I would definitely not suggest that approach.

Upvotes: 1

Related Questions