C++ Kernel Factory class

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 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;

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 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:

StackExchange Code Review Q#11592, answer score: 2