Applying chain responsibility with IFilter

Question

I have an interface IFilter and more types of filters: Filter1, Filter2, Filter3; that I want to call one after the other and if the result of one is empty, then I want to stop the process. For this I have thought of:

class FiltersSequence
{
private:
 std::vector< IFilter > m_filters;
public:
 FiltersSequence();
 ~FiltersSequence();
 /**/
 ObjectToFilter execute(const ObjectToFilter& objectToFilterIn);
};

With the implementations:

FiltersSequence::FiltersSequence()
{
 m_filters.push_back(Filter3());
 m_filters.push_back(Filter2());
 m_filters.push_back(Filter1());
}
FiltersSequence::~FiltersSequence() {}
ObjectToFilter FiltersSequence::execute(const ObjectToFilter& objectToFilterIn)
{
 ObjectToFilter filterResult = objectToFilterIn;
 for (std::size_t i = 0; i < m_filters.size(); i++)
 {
 filterResult = m_filters[i].execute();
 if (filterResult.isEmpty())
 {
 return filterResult;
 }
 }
 return filterResult;
}

It should be vector of pointers, because the filters are not all the same size; so I have changed to:

std::vector< std::shared_ptr< IFilter > > m_filters;

and

m_filters.push_back(std::shared_ptr< IFilter >(new FilterX()));

I thought of something like having a map< std::string, IFilter > for being sure that the sequence is in the order that I want. What do you think of this? Is it good enough? Or this is just making it more complicated and I need just to pay attention at push_back. BTW, is this going to call them in right order (1, 2, 3)?

Answer 1

Like Kumar have already mentioned, just keep it simple and use std::vector. It is a very efficient data structure since every element is stored next to each other in contiguous memory. This gives you great cache utilization for sequential access (which is what you do in the loop).

As for the ordering, right now you are doing

FiltersSequence::FiltersSequence()
{ // m_filters: {}
 m_filters.push_back(Filter3()); // m_filters: {Filter3}
 m_filters.push_back(Filter2()); // m_filters: {Filter3, Filter2}
 m_filters.push_back(Filter1()); // m_filters: {Filter3, Filter2, Filter1}
}

which is in reverse order since what you want is {Filter1, Filter2, Filter3}. You have to reverse the push_back calls to get the correct behaviour.

Also, you are overriding filterResult in every iteration

filterResult = m_filters[i].execute(); // filterResult is overriden.

Don't you mean to do something like

filterResult = m_filters[i].execute(filterResult);

instead? I don't have the signature of an IFilter so it is hard to know.

Beside those semantic issues, the code has good overall structure. Good job! I only have comments on the details. I'm going to assume that you use C++03 in the following:

• Drop the destructor ~FiltersSequence(). Your compiler will automatically generate one for you which does the right thing.

• Break the loop instead. I.e.,

  for (std::size_t i = 0; i < m_filters.size(); i++)
  {
   filterResult = m_filters[i].execute();
   if (filterResult.isEmpty()) break;
  }
  

  This simplifies the code and is semantically equivalent since the return statement in the bottom of the function is reached instead.

• Prefer pre-increment to post-increment. There is no semantic difference but ++i may be faster and never slower than i++ for simple types.

If you have a C++11 compiler then you can:

• Use a range-based for loop and write

  for (const auto& filter : m_filters) {
   filterResult = filter.execute();
   if (filterResult.isEmpty()) break;
  }
  

• Initialize m_filters with a std::initializer_list directly in the member initializer list

  FiltersSequence::FiltersSequence()
   : m_filters{Filter1(), Filter2(), Filter3()}
  {}
  

  Note that this also makes the order of the elements clear to the user.

Let me know in the comments if you have any further questions or need some elaboration on my comments.

• sop: Thanks a lot, I use the C++11, so I will do the updates linked to it. Shall I remove also the empty ctor (if no other intentions were taken in consideration, like no implicit copyctor, etc)?
• sop: And another question linked to the destructors: what if the IFilter has a virtual destructor?
• Frederik Aalund: @sop, I'm glad it helped. Yes, you can safely remove an empty constructor and the compiler will generate one for you. The rule of three/five/zero will help you in this kind of decision making. The use of virtual destructors is explained here. The implicitly defined destructor of FilterSequence will call the destructor of std::vector<Ifilter> which in turn will call the destructor of Ifilter (virtual or not).
• sop: I am sorry about this, but I have not managed to make it run :D. It needs to be pointer of IFilter. Using shared_ptr leads to error because of the pure abstract execute(filterResult). Any suggestions?
• Frederik Aalund: If you post a sample that compiles with an online compiler then I might be able to find the bug. That is, I need to see the remaining code to help you out with specific details.

Answer 2

I thought of something like having a map< std::string, IFilter> for being sure that the sequence is in the order that I want. What do you think of this?

std::map for such simple case should be avoided. std::map would be more appropriate if you are not sure that your IFilter objects could be inserted in any order by client code and you do have lots of entry. Of course we should measure our program performance by both(.i.e. std::vector or std::map

Is it good enough? Or this is just making it more complicated and I need just to pay attention at push_back.

Yes, it look ok. We should always use more simpler/efficient data structure(std::vector) as our default container unless we have valid reason to do otherwise.

BTW, is this going to call them in right order (1, 2, 3)?

No, It would be in order of (3,2,1) as std::vector would push_back. We can see that documentation clearly mention like this about push_back

Appends the given element value to the end of the container.

[Filter3()][Filter2()][Filter1()]
 0 1 2

• sop: I think the last YES should be a NO, I have verified and it seems that std::vector is like first in on first index, so the order is Filter3, Filter2, Filter1 (3, 2, 1). Or is it dependent on the compilator?
• Mantosh Kumar: @sop: Sorry it was my mistake and you are correct. My mistake. I have updated it my post.
• sop: No worry, but I have accepted the other answer.
• Mantosh Kumar: @sop: Information should be correct as it may be refereed by others in future. Hence I corrected. :)