Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I currently have code like this to display some objects that meet some criteria in a grid:

// simplified
void MyDialog::OnTimer()
{
    UpdateDisplay();
}

void MyDialog::UpdateDisplay()
{
    std::list<MyClass *> objects;
    someService_->GetObjectsMeetingCriteria(objects, sortField, sortDirection);

    for(std::list<MyClass *>::iterator it = objects.begin();
        it != objects.end() ; it ++)
    {
        AddObjectToGrid(*it);
    }

}   

This works fine. Now the requirement has come in to process these objects when they meet some criteria. The information on the objects can change quickly so it would be most desirable to check if an object meets criteria and then process it immediately and continue on in this fashion.

My question is how to best architect this to handle the display and processing. I could simply add a method to process the object such as:

for(std::list<MyClass *>::iterator it = objects.begin();
    it != objects.end() ; it ++)
{
    ProcessObject(*it);
    AddObjectToGrid(*it);
}

However ideally I'd check if an object meets the criteria immediately before processing it to ensure its acting on the most recent info. In this example, all the objects are checked to see if they match the criteria and then afterwards each one is processed.

Also I'm concerned this is coupling the processing code with the display code although I'm not sure how to separate them or if it's even necessary. I could solve that like this:

void MyDialog::OnTimer()
{
    ProcessObjects();
    UpdateDisplay();
}

But then I'm iterating over the list of objects twice, once to process and once to display.

Finally I could do something like:

// simplified
void MyDialog::OnTimer()
{
    ProcessAndDisplayObjects();
}

void MyDialog::ProcessAndDisplayObjects()
{
    std::list<MyClass *> objects;
    someService_->GetAll(objects, sortField, sortDirection);

    for(std::list<MyClass *>::iterator it = objects.begin();
        it != objects.end() ; it ++)
    {
        if(someService->MeetsCriteria(*it))
        {
          ProcessObject(*it);
          AddObjectToGrid(*it);
        }
    }
}   

Overall what's holding me up is I'm worried about coupling the display and processing code together and the code running efficiently because timely processing is critical. How can I best structure this code?

share|improve this question
add comment

2 Answers

up vote 1 down vote accepted

I'll describe the ideal first, then some good enoughs within the constraints you might have. Ideally you want the processing code to be outside the dialog class, you want the processing to happen separately on a thread other than the gui thread, for the processing code to get notified when the data changes so that processing is needed, and then to notify the dialog to update the data rather than using a timer. (Timers have a problem of either being too granular or not granular enough, too much and they eat up cycles unnecessarily, not enough and the GUI is unresponsive.)

OK, so you probably aren't prepared to do separate threads and notifications, but it is still important to move the processing code outside the dialog. Your concern about coupling the two processes is well founded, even if it turns out to be a trivial issue for now, requirements and code change. This is your best approach for decoupling them, and it makes adding any of the other above features later much easier. The important thing, even if the timer spawns both the processing and the update, is for the update to do as little as possible, i.e., call some function to get a value to put in the grid, and nothing else. The processing should all be done by the time that starts. Assuming no other structural changes to your code, I'd go with something like:

OnTimer()
{
  if (!processor.IsReady())
    processor.process();
  UpdateDisplay()
}

Later, if you want to spawn a new thread for the process() call, it is easy to do by calling processthread.start() or whatever instead of processor.process(); and returning, letting the next timer cycle pick up the data. If you want to have a different way of launching the process routine (a notifier that the data has changed, or a second timer, or something), you can change the above to:

OnTimer()
{
  if (processor.IsReady())
    UpdateDisplay();
}

Don't worry about iterating twice, unless the data set is really huge. And if it is huge enough to worry about, all the more reason to look at doing it outside the GUI thread, because no matter how you factor it, it will cause your display to hiccup. If your processor object is behind an interface, all the better, then you can refactor that later without affecting the dialog code. And you can add code in OnTimer() to check a flag to see if not only is the data processed, but has it changed since your last update, and skip it if not.

share|improve this answer
    
+1: I like this answer. I'm pondering how it can fit in with my application but I think you're right on. –  User Oct 13 '11 at 4:52
add comment

Since you're (at least apparently) using C++, I'd try to write the code to fit a little more closely with how the standard iterators and algorithms do. I'd start with a small facade to give your "grid" an iterator interface:

// warning: untested code.
struct grid_proxy { 
    grid_proxy &operator=(MyClass const &item) { 
        AddObjectToGrid(item);
        return *this;
    }
};

class grid_iterator : public std::iterator<std::output_iterator_tag, MyClass> { 
public:
    operator*() { return grid_proxy(); }
    grid_iterator operator++() { return *this; }
    grid_iterator operator++(int) { return *this; }
};

Then I'd write a small algorithm that probably should be in the library, but isn't: a transform_if:

template <class InputIterator, class OutputIterator, class UnaryOperation, class Predicate>
void transform_if(InputIterator start, InputIterator stop, 
                  OutputIterator out, 
                  UnaryOperation op, 
                  Predicate pred) 
{
    while (start != stop) {
        if (pred(*start))
            *out++ = op(*start);
        ++start;
    }
}

With these, your overall operation becomes something like:

std::list<MyClass *> objects;
someService_->GetAll(objects, sortField, sortDirection);

transform_if(objects.begin(), objects.end(), grid_iterator(), MeetsCriteria());

At the moment, I've assume that MeetsCriteria will be written as a functor rather than a member function. If you can use C++11 features, you might want to consider using a lambda instead.

In any case, this provides pretty fair de-coupling of most of the code. transform_if simply knows that it's writing to an iterator -- only the grid_iterator has any clue that AddObjectToGrid is involved. A different iterator can produce the output entirely differently.

Likewise, transform_if doesn't really care at all about the criteria or how it's determined, as long as it can be invoked like a function with the right signature.

While this does impose a little overhead in writing a conforming iterator type and such, it has a few advantages. The aforementioned decoupling is certainly handy. The fact that anybody who knows the C++ library reasonably well will know what's going on almost without a second thought is also a very good thing as a rule.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.