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

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.