Ugly class interface definition

Question

The function of the class:

1. Receive a sequence of image frames, the sequence is infinite.
2. Detect if there is motion in the frames.
3. Group motion frames according to certain algorithm.

So far the design is(pretty silly):

class MotionDetector
{ 
      //detect motion in the frame, return true if the group is captured. 
      //frameToDispose is the frame that need be dispose, or for further process.
      public bool ProcessFrame(Frame in, out frameToDispose); 
    }

The consumer(snippet):

public void Foo()
{
     bool groupCaptured = motionDetector.ProcessFrame(nextFrame, out lastFrame);

    if (IsStaticFrame(lastFrame)) { lastFrame.Dispose(); }
    else { imagesArray.Add(lastFrame); }

    if(groupCaptured) { processImageGroup(imagesArray);  }
}

I feel uncomfortable with the MotionDetector's design of the following:

1. The way to get the images group.
2. The way to dispose motionless frame.
3. The way to notify client that group captured.

Can you give some advice on the class's interface design, so that it is easier and more elegant for the client to consume this class?

Answer 1

If I understood correctly your question, you don't like the way a client of your class must use the method you are providing...
What about making the frame to dispose a property of the class instead of an out parameter?

class MotionDetector{ 

  public bool PreProcessFrame(Frame in); 

  public Frame frameToDispose{
    get;        
  }      
}

Then you could use it like:

bool groupCaptured = motionDetector.ProcessFrame(nextFrame);
if (IsStaticFrame(motionDetector.frameToDispose)){
  // ...
}

Otherwise (if it makes sense for your application) you might do like this:

class MotionDetector{       
  // returns frame to dispose if sucessful, null otherwise
  public Frame PreProcessFrame(Frame in); 
}

EDIT about letting consumer know the group captured, using an event as suggested in the comments:

class GroupCapturedEventArgs : EventArgs{
  // put relevant information here...
}
class MotionDetector{
  public event EventHandler<GroupCapturedEventArgs> GroupCaptured;
  // then somewhere in your code:
  private vois SomeMethod() {
    // a group captured
    if (GroupCaptured != null) {
      GroupCaptured (this,new GroupCapturedEventArgs(/*whatever*/));
    }
  }      
}

Answer 2

The consumer class is doing the work that MotionDetector should be doing. Perhaps the MotionDetector contructor (or some method in the class) should take a stream of frames and this work should be done internally. The class should expose only the requisite image array after the algorithm has been run.

Answer 3

I'd probably do something like this:

public class MotionDetector
{
    private IFrameGroupListener m_listener;

    public MotionDetector(IFrameGroupListener listener)
    {
        m_listener = listener;
    }

    public void NewFrame(Frame f)
    {
        if(DetectMotion(f))
        {
            var group = GetCaptureGroup();
            m_listener.ReceiveFrameList(group);
        }
    }
}

public interface IFrameGroupListener
{
    void ReceiveFrameList(IList<Frame> captureGroup);
}

public class FramePump
{
    private MotionDetector m_detector;

    public FramePump(MotionDetector detector)
    {
        m_detector = detector;
    }

    public void DoFrame()
    {
        Frame f = GetFrameSomehow();
        m_detector.NewFrame(f);
    }

}

I'm assuming that DetectMotion() stores the frame, otherwise you'd have to keep it in a pending list until it was time to get rid of it. Anyway, the FramePump gets individual frames from the actual device/file. That's it's job. The MotionDetector is responsible for detecting motion, and passing groups of frames with motion in them to the FrameGroupListener, which then does whatever it needs to do.

This way, the classes are nicely separated with responsibilities, and very little is done in a stateful way - all state is localized to the individual classes. Since the calls are all void, they can be dispatched to arbitrary threads if you need to.

The FramePump probably gets triggered on a timer loop of some sort.

I'd probably consider breaking the grouping algorithm into a separate class also - have the motiondetector class spit out each frame along with a bool indicating whether or not motion was detected, and then the MotionGrouper class would take those in individually, and spit out lists of frames according to whatever algorithm is desired. 'Detecting motion' and 'determining how to group frames' are pretty clearly two responsibilities. But, it should be clear how you'd do that refactoring in this general kind of pipeline design.