##EDIT
EDIT
##EDIT
EDIT
OK. I hate getter/setters they totally break encapsulation (OK I exagerate but they are totally overused by beginners 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.
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.
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.
##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.
##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.