A while ago, I found in a website some code examples of utility functions that are used when creating, destructing objects, or even when overloading some of their operators.
More precisely, the following member functions are mainly used: init, copy, set, and destroy.
- The
init member function is used to initialize all the private members. It's mostly called inside the constructors, e.g. the default or parameter constructor.
- The
copy member function is used to do a deep copy of an object passed as a const reference. It is called inside the reference constructor and the overload of the operator =.
- The
set member function which mainly allocates memory for the private members that require it.
- Finally, the
destroy member function is used for releasing the allocated memory. It's called, for example, inside of the destructor.
I would like to have your opinion and know if this is a good programming practice? Which are the benefits or drawbacks? Any comments and suggestions are welcomed!
Below, I'm illustrating how those member functions are defined for a CMatrix<T> class.
matrix.h
template < class T >
class CMatrix{
CMatrix(){ this->initMatrix(); }
CMatrix(int nRows, int nCols, int nChannels){
this->initComplexMatrix();
this->setComplexMatrix(nRows, nCols, nChannels);
}
CMatrix(const CMatrix<T> & refMatrix){
this->initComplexMatrix();
this->copyComplexMatrix(refMatrix);
}
CMatrix<T> & operator = (const CMatrix<T> & refMatrix){
if(this!=&refMatrix){
this->destroyComplexMatrix();
this->initComplexMatrix();
this->copyComplexMatrix(refMatrix);
}
return (*this);
}
T & CMatrix<T>::operator()(int, int, int);
T CMatrix<T>::operator()(int, int, int) const;
......
void initMatrix();
void copyMatrix(const CMatrix<T> & );
void setMatrix(int, int, int = 1);
void destroyMatrix();
......
~CMatrix(){ this->destroyMatrix(); }
private:
T *** m_pData;
int m_nRows;
int m_nCols;
int m_nChannels;
};
matrix.cpp
#include <matrix.h>
template < class T >
inline T & CMatrix<T>::operator()(int mrow, int mcol, int mchannel){
assert(mrow >= 0 && mrow < this->getRows());
assert(mcol >= 0 && mcol < this->getCols());
assert(mchannel >= 0 && mchannel < this->getChannels());
return this->m_pData[mrow][mcol][mchannel];
}
template < class T >
void CMatrix<T>::initMatrix(){
this->m_nRows = 0;
this->m_nCols = 0;
this->m_nChannels= 0;
this->m_pData = NULL;
}
template < class T >
void CMatrix<T>::copyMatrix(const CMatrix<T> & refMatrix){
if(refMatrix.m_pData!=NULL){
this->setMatrix(refMatrix.getRows(), refMatrix.getCols(), refMatrix.getChannels());
for(register int dy=0; dy < this->getRows(); dy++){
for(register int dx=0; dx < this->getCols(); dx++){
for(register int ch=0; ch < this->getChannels(); ch++){
this->m_pData[(dy)][(dx)][(ch)] = refMatrix.m_pData[(dy)][(dx)][(ch)];
}
}
}
}
else{
this->m_pData = NULL;
}
}
template < class T >
void CMatrix<T>::setMatrix(int nRows, int nCols, int nChannels){
this->destroyMatrix();
this->m_pData = NULL;
this->m_pData = new T ** [nRows];
for(register int dy=0; dy < nRows; dy++){
this->m_pData[dy] = NULL;
this->m_pData[dy] = new T * [nCols];
for(register int dx=0; dx < nCols; dx++){
this->m_pData[dy][dx] = NULL;
this->m_pData[dy][dx] = new T[nChannels];
}
}
this->setRows(mrows);
this->setCols(mcols);
this->setChannels(mchannels);
}
template < class T >
void CMatrix<T>::destroyMatrix(){
if(this->m_pData!=NULL){
for(register int dy=0; dy < this->getRows(); dy++){
for(register int dx=0; dx < this->getCols(); dx++){
delete [] this->m_pData[dy][dx];
}
delete [] this->m_pData[dy];
}
delete [] this->m_pData;
this->m_pData = NULL;
}
}
No, this is not recommended. The way you propose is not exception safe and is not compatible with const or subobjects that need non-default construction.
Instead use the ctor-initializer-list. Code reuse can be achieved through static helper functions called in the ctor-initializer-list or by moving logic into subobject constructors.
For memory allocation, use a subobject per resource. The memory management logic ends up in the subobject constructor and destructor. In many cases, you can use existing RAII classes from the library, such as std::vector, and not need to write any memory management code yourself.
Most operators can reuse the logic in the constructors, using the copy-and-swap idiom.
EDIT: Exception-safe construction might look something like this:
#include <vector>
template<typename T>
class matrix
{
int m_nCols;
std::vector<T*> m_rows;
std::vector<T> m_cells;
size_t makeIndex( int row, int col ) const { return row*m_nCols + col; }
public:
matrix( int nRows, int nCols )
: m_nCols(nCols), m_rows(nRows), m_cells(nRows * nCols)
{
while (nRows--) m_rows[nRows] = &m_cells[nRows * nCols];
}
matrix( const matrix<T>& other )
: m_nCols(other.m_nCols), m_rows(other.m_rows.size()), m_cells(other.m_cells)
{
int nRows = other.m_rows.size();
while (nRows--) m_rows[nRows] = &m_cells[nRows * nCols];
}
void swap( matrix& other )
{
using std::swap;
swap(m_nCols, other.m_nCols);
swap(m_rows, other.m_rows);
swap(m_cells, other.m_cells);
}
matrix& operator=( matrix other )
{
other.swap(*this);
return *this;
}
const T& operator()( int row, int col ) const { return m_cells[makeIndex(row,col)]; }
T& operator()( int row, int col ) { return m_cells[makeIndex(row,col)]; }
};
The std::vector destructor will take care of freeing the memory, and since the two allocations are separate objects, if m_cells fails to allocate its memory, the destructor for m_rows will run and nothing will leak.
Of course, std::vector is a little bit overkill here, a fixed-size array RAII class would be sufficient. But std::auto_ptr can't be used with arrays. I think C++0x is supposed to add a standard fixed-size RAII array class.
EDIT: Per request, a 3-D version:
#include <vector>
template<typename T>
class cube
{
int m_nCols, m_nRows;
std::vector<T> m_cells;
size_t makeIndex( int row, int col, int channel ) const { return (channel*m_nRows + row)*m_nCols + col; }
public:
cube( int nRows, int nCols, int nChannels )
: m_nCols(nCols), m_nRows(nRows), m_cells(nRows * nCols * nChannels)
{
}
cube( const cube<T>& other )
: m_nCols(other.m_nCols), m_nRows(other.m_nRows), m_cells(other.m_cells)
{
}
void swap( cube& other )
{
using std::swap;
swap(m_nCols, other.m_nCols);
swap(m_nRows, other.m_nRows);
swap(m_cells, other.m_cells);
}
cube& operator=( cube other )
{
other.swap(*this);
return *this;
}
const T& operator()( int row, int col, int channel ) const { return m_cells[makeIndex(row,col,channel)]; }
T& operator()( int row, int col, int channel ) { return m_cells[makeIndex(row,col,channel)]; }
class channel_iterator
{
cube& const cube;
int const row, col;
int channel;
friend class cube;
channel_iterator( cube& all, int r, int c, int n ) : cube(all), row(r), col(c), channel(n) {}
public:
T& operator*() const { return cube(row, col, channel); }
channel_iterator& operator++() { ++channel; return *this; }
channel_iterator operator++(int) { return channel_iterator(cube, row, col, channel++); }
bool operator!=(const channel_iterator& other) const { assert(&cube == &other.cube); return (row == other.row && col == other.col && channel == other.channel); }
};
int channel_count() const { return m_cells.size() / m_nRows / m_nChannels; }
pair<channel_iterator, channel_iterator> range(int row, int col) { return make_pair(channel_iterator(*this, row, col, 0), channel_iterator(*this, row, col, channel_count())); }
};
A better pattern for implementing assignment is the copy and swap idiom.
X& X::operator= (const X& rhv)
{
X copy(rhv); //reuse copy constructor
this->swap(copy); //reuse swap method - also useful for the user of the class!
//previously held resources automatically released by the destructor of copy
}
This performs the necessary steps in an order that keeps left-hand value unchanged if an exception occurs during the copying: doesn't release resources before new resources have been successfully obtained.
And the advantage over the methods you have is that the swap method is useful not only internally for implementing the class, but also for the user of the class. In case of the CMatrix class the implementation would be:
void CMatrix<T>::swap(CMatrix<T>& other)
{
std::swap(m_pData, other.m_pData);
std::swap(m_nRows, other.m_nRows);
std::swap(m_nCols, other.m_nCols);
std::swap(m_nChannels, other.m_nChannels);
}
Also, if suitable, it is a better idea to reuse existing RAII classes instead of managing memory manually in each and every class.
