2

I'm implementing a simple data reader to get image sequences from either a pre-recorded video file, or a camera device. 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;

// base class
class DataReader {
  friend datareader_ptr make_data_reader(const std::string& file_path);
public:
  virtual bool get_next_frame(Frame& frame)=0;
  virtual ~DataReader() {}
protected:
  // “pipeline” is concerned with some libraries I use to read from file or camera
  DataReader(pipeline pipe);
  pipeline pipe_;
};

// Derived class 1, reading from file
class FileDataReader : public DataReader {
  friend datareader_ptr make_data_reader(const std::string& file_path);
public:
  virtual bool get_next_frame(Frame& frame) {
    // code for reading a frame from file
  }
  virtual ~FileDataReader() {}
protected:
  FileDataReader(pipeline pipe):DataReader(pipe){};
};

// Derived class 2, reading from camera
class CameraDataReader : public DataReader {
  friend datareader_ptr make_data_reader(const std::string& rfile_path);
public:
  virtual bool get_next_frame(Frame& frame) {
    // reading frame from camera
  }
  virtual ~CameraDataReader() {}
protected:
  CameraDataReader(pipeline pipe):DataReader(pipe) {}
};

// The factory function that creates the reader.
// The user is only allowed to use this function to create the reader instances.
datareader_ptr make_data_reader(const std::string& file_path) {
  pipeline pipe;
  if (file_path=="") {
    // set up pipe to read from camera
    return datareader_ptr(new CameraDataReader(pipe));
  } else {
    // set up pipe to read from file
    return datareader_ptr(new FileDataReader(pipe));
  }
}

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!

6
  • 2
    If you already have a working code perhaps you can use Code Review (read their help center carefully before asking)
    – user202729
    Jan 6, 2021 at 4:16
  • Seems like a sensible usage to me: common base class and sub classes with similar interface but different implementation. My 2c: I hate code like int fn() { if (cond) { return 1; } else { return 0; } } - without checking the code thoroughly how can I be sure there isn't a case with no return ?...
    – John3136
    Jan 6, 2021 at 4:26
  • @John3136 Any suggestions on revising the if-else part ?
    – zhouch
    Jan 6, 2021 at 4:37
  • There are not many wokarounds around an if/else test. You should consider using make_shared(), using new aa a fonction parameter can lead to memory leaks, you should avoid doing that, as a matter of course. Jan 6, 2021 at 6:01
  • @zhouch int fn() { if (cond) { return 1; } return 0; } - it's a style thing - but it can also prevent compiler warnings depending how smart the compiler is.
    – John3136
    Jan 6, 2021 at 8:37

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Browse other questions tagged or ask your own question.