Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have an class Contours that contains a vector of contours. I use OpenCV to store the contours, and I have another class Filters where I have some functions that apply filters on those contours (like length, or convexity).

Here is a similar example:

class Contours
{
private:
  std::vector< std::vector< cv::Point > > m_contours;

public:
  Contours() {}
  ~Contours() {}

  std::vector< std::vector< cv::Point > > getContours() const
  { return m_contours; }
  void setContours(std::vector< std::vector< cv::Point > >& contoursIn)
  { m_contours = contoursIn; }
};

class Filters
{
private:
  double m_minLen;
  double m_maxLen;

public:
  void filterLen(const Contours& cIn, Contours& cOut)  
  {
    std::vector< std::vector< cv::Point > > c0, c1 = cIn.getContours();
    for (size_t i = 0; i < c1.size; i++)
    {
      if (length(c1[i]) > m_minLen && length(c1[i]) < m_maxLen)
        c0.push_back(c1[i]);
    }
    cOut.setContours(c0);
  }
  void filterConvex(const Contours& cIn, Contours& cOut) // the same way
};

I want some suggestions of optimisations. I would like them in the idea of changing the functions to return a Contours object, so I can use the filters in a cascade way:

Contours filtered = fltrs.filterLen(fltrs.filterConvex(contours));

How should I proceed? Shall I return a reference of Contours or the object itself?

Contours& filterLen(const Contours& cIn)  
{
  std::vector< std::vector< cv::Point > > c0, c1 = cIn.getContours();
  for (size_t i = 0; i < c1.size; i++)
  {
    if (length(c1[i]) > m_minLen && length(c1[i]) < m_maxLen)
      c0.push_back(c1[i]);
  }
  cIn.setContours(c0);
}
share|improve this question

closed as off-topic by Jerry Coffin, Vogel612, unholysampler, RubberDuck, Marc-Andre Jul 7 at 13:14

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – Vogel612, Marc-Andre
  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – Jerry Coffin, unholysampler, RubberDuck
If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers 2

up vote 4 down vote accepted

You should return the object itself, thereby utilizing Return Value Optimization (RVO). Modern compilers should be able to support this, allowing them to optimize out the extra copy. You could still test both methods if you'd like to be sure, but this is usually how it's done nowadays.

In addition, returning by value will avoid issues stemming from returning by reference to a local variable, plus it doesn't need to be modified elsewhere.

Misc.:

  • Use std::size_t, which is specific to C++ as it is in the std namespace.

  • Since you're not defining the destructor, just leave it out; the compiler will provide one for you.

  • Consider having a typedef for you 2D vector so that you don't have to write it out each time:

    typedef std::vector<std::vector<cv::Point > > SomeShorterName;
    
  • Try to avoid the setters and getters if possible, which are not good for encapsulation.

    If you just need an object with an initial value, overload the default constructor. If the calling code doesn't need the value of m_contours (if it can just be displayed), then eliminate the getter.

    You can replace the default constructor with an initializer list:

    Contours(std::vector<std::vector<cv::Point > > const& contoursIn) : m_contours(contoursIn) {}
    

    The argument is passed by const& since it isn't (and shouldn't be) modified.

share|improve this answer
    
Thanks. I have done the typedef in the code. I will use std::size_t, but size_t isn't it the same (without namespacing)? –  thedarkside ofthemoon Jul 4 at 15:30
1  
@thedarksideofthemoon: Yes, it's still the same type, but keeping it in the namespace can avoid name-clashing if there's a use of the non-namespaced size_t. –  Jamal Jul 4 at 15:31
    
About the getter, there is an error, I'll edit the question –  thedarkside ofthemoon Jul 4 at 15:33

I'd step back and think about why you (want to) have a Contours class for a moment.

Right now, it appears that the Filters class depends on Contours::getContours returning a vector, and it knows about all the details of what's stored in that vector.

Likewise, Contours::setContours requires that the caller knows that it contains a std::vector<std::vector<cv::Point> >. At least as you've posted it, the Contours class doesn't seem to really do anything beyond that.

In short, it appears that something like:

typedef std::vector<std::vector<cv::Point> > Countours;

...would accomplish pretty much the same thing as your current class--but would completely avoid the questions around the efficiency (or lack of it) of getContours and/or setContours.

That doesn't immediately strike me as the best possible design though. My immediate reaction would be that Contour would be a more useful type. I'd probably start with that. If client code deals a lot with collections of contours, it might make sense to define a class for that as well, but only when/if you've defined more for it to do than "act like a vector, but with clumsier syntax."

You can also clean up your Filters code quite a bit by using standard algorithms. For example, your filterLen would work out as an invocation of std::copy_if:

void filterLen(Contour const &cIn, Contour &cOut) {   
    cOut.clear();
    std::copy_if(cIn.begin(), cIn.end(),
        std::back_inserter(cOut),
        [min_len, max_len](Contour const &c) { 
            return c.length() > min_len && c.lengt() < max_len; 
        });
}

One other point: although technically allowable, cIn and cOut strike me as rather poor names to use. They differ only in capitalization from cin and cout, which are defined by the standard library and very widely used. Especially to people accustomed to case-insensitive languages, this could lead to considerable confusion. Depending on font, it could also be difficult to distinguish even for those accustomed to cast-sensitive languages.

share|improve this answer
    
Ah, I should've known that Contours was hardly doing anything. My suggestions would've rendered it even more needless. –  Jamal Jul 4 at 16:01
    
In fact Contours, is containing more than just that vector, it contains also its bounding box, and some other infos about it. The idea of my question was how to return an object (more efficiently). The idea of using lambda functions and it is nice, thanks. –  thedarkside ofthemoon Jul 7 at 7:24
2  
@thedarksideofthemoon: This is why the rules demand that you post real code. We can't provide meaningful reviews when you've left out crucial parts of the code. –  Jerry Coffin Jul 7 at 11:29

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