2
\$\begingroup\$

I'm novice in C++, I've recently read some simple books and now I'm reading Scott Meyers "Effective C++" book, but I understand that I can't apply new knowledge.

I have to realize a simple C++ class that (with help of openCV) generates bitmap kernels consist of n circles.

Here is my code: Link to source on gist.github.com

Kernel class is a wrapper for cv::Mat kernels. Just for simpler access to center, radius and data. KernelFactory is class that generates kernels (double-circle kernel, one-circle kernel or n-circle kernel).

I need a piece of advice about how correctly declare class members and methods? Do I need to keep some members as references, or no?

UPDATE 1:

According to the advice and comments of @Loki Astari I've made some changes in my first variant of code, and here is new version of code, with a few questions, written as Notes in comments in code.

/// Calculating the max value of array
int maxArr(std::vector<int> const& arr)
{
  return *std::max_element(arr.begin(), arr.end());

  // Note 0.
  // Maybe, it's preffered to write something like:
  //    int max = ....;
  //    return max;
  // ? 
}


/**
 * Wrapper for cv::Mat kernel
 */
class Kernel
{
  private:

    // Data (array) of kernel
    cv::Mat data;

  public:

    // Note 1. 
    // Constructor gets const cv::Mat array by value
    // Should I use `by reference`? Using copying by value, it means
    // that creating Kernel instance i have array of cv::Mat in 2 copies:
    // 1) where it was created before constructing,
    // 2) after constructing in class member `data`

    Kernel(const cv::Mat kernel) 
    {
      data = kernel;
    }


    /**
     * Getters
     */
    cv::Mat const& getData() const { return data; }

    cv::Mat& getData() { return data; }

    int getSize() const { return data.rows; }

    int getRadius() const { return (data.rows-1)/2; }

    cv::Point getCenter() const 
    {
      int R = getRadius();

      cv::Point center(R-1, R-1);
      return center;
    }

    // Note 2. Now, am I going in right direction using the getters? ;)
};

/**
 * Factory for Kernels
 */
class KernelFactory
{
  private:

    static int const POINT_VALUE = 255;

    /// Draw a circle on Mat kernel in some center of radius r
    void draw_circle(cv::Mat& kernel, const cv::Point2f center, int r) const
    {
      int c = ceil ( (r-2)/2 );
      int d = c+1;

      for (int ci=-c; ci<=c; ci++) {
        kernel.at<char>(center.y + (r-1), center.x + ci) = POINT_VALUE;
        kernel.at<char>(center.y - (r-1), center.x + ci) = POINT_VALUE;
        kernel.at<char>(center.x + ci, center.y + (r-1)) = POINT_VALUE;
        kernel.at<char>(center.x + ci, center.y - (r-1)) = POINT_VALUE;
      }
      for (int di=1; di<=d; di++) {
        kernel.at<char>(center.y + (r-1-di), center.x + di + c) = POINT_VALUE;
        kernel.at<char>(center.y - (r-1-di), center.x + di + c) = POINT_VALUE;
        kernel.at<char>(center.x - di - c, center.y - (r-1-di)) = POINT_VALUE;
        kernel.at<char>(center.x + di + c, center.y - (r-1-di)) = POINT_VALUE;
      }
    }

    /// Generic function for building n-size circle mask
    /// This function is called by other public facades
    Kernel build_circle_mask_n(std::vector<int> const& radiuses)
    {

      int n           = radiuses.size();
      int Rmax        = maxArr(radiuses);
      int kernel_size = Rmax*2-1;

      cv::Mat kernel   = cv::Mat::ones(kernel_size, kernel_size, CV_8U);
      cv::Point center(Rmax-1, Rmax-1);

      for (int i=0; i<n; i++) {
        draw_circle(kernel, center, radiuses[i]);
      }

      return Kernel(kernel);

      // Note 3:
      // Is this resource-safe of returning instance of Kernel in this situation?
    }

  public:

    /// Build n-circle mask kernel
    Kernel circleMaskN(std::vector<int> const& radiuses)
    {
      return build_circle_mask_n(radiuses);
    }

    /// Build Double-circle mask kernel
    Kernel circleMask2(int r, int R)
    {
      std::vector<int> radiuses(2);
      radiuses[0] = r;
      radiuses[1] = R;

      return build_circle_mask_n(radiuses);
    }

    /// Build uni-circle kernel
    Kernel circleMask(int r)
    {
      std::vector<int> radiuses(1);
      radiuses[0] = r;
      return build_circle_mask_n(radiuses);
    }

    // Note 4: 
    // Should I overload circleMaskN method for a case
    // when radius-vector is of size = 1?
    // Maybe I should write a version of build_circle_mask_n
    // that gets integer parameter, but not vector(1)?
};

If you prefer gist syntax much, here is a gist link to this source.

\$\endgroup\$
2
\$\begingroup\$

The first thing that I notice is:

int maxArr(int arr[], int length)

Notice that you have to pass an array and a length (as the size is not part of the array). In C++ we usually fix this by using std::vector (or std::array in C++11). Pass this object around as it contains the size information.

Also notice that arrays actually decay to pointers. And in this function the content of the array is not modified (or it looks like the content should not be modified). Thus you really want to pass a const version to indicate your intention. This will also prevent accidental mistakes in the future.

int maxArr(std::vector<int> const& arr)

Now this finds the maximum element in the array. If you look in the standard library you will notice that there is already an algorithm that does this (getting used to the standard library will save you a lot of time):

int max = *std::max_element(arr.begin(), arr.end()); // assuming arr is a vector

// note: std::max_element() returns an iterator so is only guranteed to work if
//       the container is not empty.

OK. I hate getter/setters they totally break encapsulation (OK I exagerate but they are totally overused by beginners). They basically tightly couple your code to a particular implementation of an internal representation. You may think they allow you to change the implementation this is nearly always false apart from the simplest of classes where you can transform the internal representation easily. But if the object is complex and/or expensive to create it basically traps you into a specific implementation. Your method should be actions (think of verbs) that manipulate the object. OK. Moving on.

Are you sure you want to return by value:

cv::Mat getData() const
{
  return data; 
}

Because you are returning by value you are copying the value out of the object. This is not usually what you want. You generally want to return a reference to the object inside your object (not because the getter is const you want to return a const reference).

cv::Mat const& getData() const
    //  ^^^^^^
{
  return data; 
}

If you want the getter to return an object that can be manipulated then you should also create a non const version.

cv::Mat  &  getData()
     // ^^^        Note: ^^^^^^ no const here.
{
  return data; 
}

OK. You store some values about the data object:

  size   = kernel.rows;
  radius = (size-1)/2;
  center = calc_center(radius);

Do you really need to store these values? It seems the getter could just get the value from the data object.

int getSize() const { return data.rows; }

Thus if data is modified you return the correct number of rows. I know currently in your design you can not modify data as it is not exposed. But this will future proof your code to changes that can happen in the future.

Comments are great. But they should not be explaining the code (the code does that). They should explain your intention. Then a maintainer can verify that the code implements the intention.

Perfect example of a crap comment:

// value of 1 to set in mask
int VALUE;

Yet later in the code:

/// Constructor (nothing happens)
KernelFactory(): VALUE(255) {}

Really 255 or 1 what does it mean. Do you really need to tell me it is a constructor. Do you think I am so stupid you need to tell me? I can see it is a constructor why are you wasting space with a meaningless comment.

If this is const and never modified then it should be declared as a const. Since the same thing is used in all instances of KernelFactory we may as well declare it as static:

static int const VALUE = 255;

Now you don't even need a constructor! (what do I do with the comment).

Returning a const value has no meaning.

const Kernel circleMaskN(int n, int radiuses[])
const Kernel circleMask2(int r, int R)
const Kernel circleMask(int r)

Because you are returning a value you are copying the value out of the function.

EDIT

Based on code changes:

// Note 1. 
// Constructor gets const cv::Mat array by value
// Should I use `by reference`? Using copying by value, it means
// that creating Kernel instance i have array of cv::Mat in 2 copies:
// 1) where it was created before constructing,
// 2) after constructing in class member `data`

Kernel(const cv::Mat kernel) 

Yes you should be passing by const reference here. What is happening is that you pass by value so a copy is done calling the copy constructor creating the parameter kernel. Then the during the assignment another copy is done copying kernel into data.

Also you should prefer to use the initializer list. Otherwise you will be default constructing data before using the assignment operator to copy kernal into data.

So your code is currently:

1) Copy Construct cv::Map into kernal
2) Default construct data
3) Use assignment operator from kernal into data
    This is probably another copy.

Your constructor should look like this:

Kernel(cv::Mat const& kernel)
   : data(kernal)   // Only one copy directly to data on copy construction.
{} 

Note 2. Now, am I going in right direction using the getters?

Better.

Note 3: Is this resource-safe of returning instance of Kernel in this situation?

This is fine.
If you are using C++11 you may want to look up move constructors to see if you can avoid copy the Kernel object during the return.

Note 4: Should I overload circleMaskN method for a case when radius-vector is of size = 1? Maybe I should write a version of build_circle_mask_n that gets integer parameter, but not vector(1)?

That totally depends on use case.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your comments. I've made some changes in my code (in question post). Can you look at it now, after changes, and say a few words? (There are some questions in comments in code) \$\endgroup\$ – Larry Cinnabar May 8 '12 at 18:43
  • \$\begingroup\$ Please don't edit the question like that. Questions and answers become out of sync and thus less valuable to new readers. Any changes should be additions to the question not a modification. \$\endgroup\$ – Martin York May 8 '12 at 21:50
  • \$\begingroup\$ Thank you for all your comments. I appreciate your help. About my post edit. I've added some new information, my only change of main post was that I've hidden the first variant of source code under the link - to make the post question a bit shorter. \$\endgroup\$ – Larry Cinnabar May 9 '12 at 19:30
  • \$\begingroup\$ I think that returning by value invokes the move constructor, and compiler by even elide it, are you sure is not better to return the return element by value? \$\endgroup\$ – Blasco Feb 9 '18 at 3:51
  • \$\begingroup\$ @WooWapDaBug: A return by value will invoke move if the object being returned is an r-value (or is it x-value in the new terminology). But an object member is not an r-value and thus will be copied; otherwise you would be destroying (making it undefined as the result of a move) the value in the object which is not acceptable. \$\endgroup\$ – Martin York Feb 9 '18 at 16:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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