Hough transform algorithm - Idiomatic c++ I've got some working code to compute the Hough transform for circles:


#include &lt;stdio.h&gt;
#include &lt;opencv/cv.hpp&gt;
#include &lt;math.h&gt;
#include &lt;string&gt;
#include &lt;iostream&gt;

int *** allocate3DArray(int y, int x, int r)  {
    int ***array = (int***)malloc(sizeof(int**)*y);
    for (int i = 0; i &lt; y; i++)  {
        array[i] = (int**)malloc(sizeof(int*)*x);
        for (int j = 0; j &lt; x; j++)  {
            array[i][j] = (int*)malloc(sizeof(int)*r);
            for(int k = 0; k &lt; r; k++) {
                array[i][j][k] = 0;
            }
        }
    }
    return array;

}

void free3d(int ***arr, int y, int x, int r)  {
    for (int i = 0; i &lt; y; i++)  {
        for (int j = 0; j &lt; x; j++)  {
            free(arr[i][j]);
        }
        free(arr[i]);

    }
    free(arr);

}

vector&lt;Circle&gt; HoughTransformCircles(Mat image, Mat &amp;gradient_mag, Mat &amp;gradient_dir)  {
    //Maximum circle radius is rLen = image.width / 2
    int rLen = image.rows/2;
    int ***houghSpace = allocate3DArray(image.rows, image.cols, rLen);
    for (int y = 0; y &lt; image.rows; y++) {
        for (int x = 0; x &lt; image.cols; x++) {
            for (int r = 0; r &lt; rLen; r++)     {
                int x0 = x - (int)(r*cos(gradient_dir.at&lt;double&gt;(y, x)));
                int y0 = y - (int)(r*sin(gradient_dir.at&lt;double&gt;(y, x)));
                if(x0 &gt;= 0 &amp;&amp; x0 &lt; image.cols &amp;&amp; y0 &gt;= 0 &amp;&amp; y0 &lt; image.rows )  {
                    houghSpace[y0][x0][r]++;
                }
                x0 = x + (int)(r*cos(gradient_dir.at&lt;double&gt;(y, x)));
                y0 = y + (int)(r*sin(gradient_dir.at&lt;double&gt;(y, x)));
                if(x0 &gt;= 0 &amp;&amp; x0 &lt; image.cols &amp;&amp; y0 &gt;= 0 &amp;&amp; y0 &lt; image.rows )  {
                    houghSpace[y0][x0][r]++;
                }

            }
        }
    }
    vector&lt;Circle&gt; circles;
    for (int y = 0; y &lt; image.rows; y++)    {
        for (int x = 0; x &lt; image.cols; x++)  {
            for (int r = 0; r &lt; rLen; r++)  {
                if (houghSpace[y][x][r] &gt; 30)  {
                    Circle temp  = Circle(x, y, r);
                    circles.push_back(temp);
                }
            }
        }
    }

    free3d(houghSpace, image.rows, image.cols, rLen);

   return circles;
}



How do I make this as idiomatic as possible? I am not allowed to use C++ 11  unfortunately.
Here are some suggestions for improving the code.

Use all required #includes

The code uses vector but doesn't include the corresponding header.  The code should have

#include &lt;vector&gt;


Use &lt;cmath&gt; instead of &lt;math.h&gt;

The difference between the two forms is that the former defines things within the std:: namespace versus into the global namespace.  Language lawyers have lots of fun with this, but for daily use I'd recommend using &lt;cmath&gt; (and also &lt;cstdio&gt;).  See this SO question for details. 

Avoid malloc and free

The old C-style calls still work, of course, but are not recommended.  They are error prone and tend to require more code to do correctly (i.e. checking the return value of malloc for NULL).  Instead, the modern C++ idiom is to use RAII which stand for "Resource Acquisition Is Initialization).  See R.10 and R.1 for details.

Use objects

The 3D array would be much better implemented as an object.  While you could, of course, write one (and I'd encourage you to try that as a learning exercise), there's an even smarter way to do that in the next suggestion.

Know your libraries

The code is already using the OpenCV Mat class, so what would make sense would be to also use that for the Hough space as well.  So this line:

int ***houghSpace = allocate3DArray(image.rows, image.cols, rLen);


would be written instead like this:

int dims[] = { image.rows, image.cols, rLen };
Mat houghSpace(3, dims, CV_8UC(1), Scalar::all(0));


Since that's a properly designed C++ class, it will automatically allocate memory as needed and also de-allocate it when the houghSpace object's destructor is called.  Using it is almost a simple:

houghSpace.at&lt;cv::Vec3i&gt;(y0,x0)[r]++;


Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority.  In this code, gradient_mag in HoughTransformCircles and r in free3d are unused. Your compiler is probably also smart enough to tell you that, if you ask it to do so. 

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.  It also appears you've got using namespace cv; somewhere as well and it's also not a good idea for similar reasons.

Use variables to clarify your code

The code currently has this nested set of loops:

for (int y = 0; y &lt; image.rows; y++) {
    for (int x = 0; x &lt; image.cols; x++) {
        for (int r = 0; r &lt; rLen; r++)     {
            int x0 = x - (int)(r*cos(gradient_dir.at&lt;double&gt;(y, x)));
            int y0 = y - (int)(r*sin(gradient_dir.at&lt;double&gt;(y, x)));
            if(x0 &gt;= 0 &amp;&amp; x0 &lt; image.cols &amp;&amp; y0 &gt;= 0 &amp;&amp; y0 &lt; image.rows )  {
                houghSpace[y0][x0][r]++;
            }
            x0 = x + (int)(r*cos(gradient_dir.at&lt;double&gt;(y, x)));
            y0 = y + (int)(r*sin(gradient_dir.at&lt;double&gt;(y, x)));
            if(x0 &gt;= 0 &amp;&amp; x0 &lt; image.cols &amp;&amp; y0 &gt;= 0 &amp;&amp; y0 &lt; image.rows )  {
                houghSpace[y0][x0][r]++;
            }
        }
    }
}


Instead, I'd write it like this:

for (int y = 0; y &lt; image.rows; y++) {
    for (int x = 0; x &lt; image.cols; x++) {
        for (int r = 0; r &lt; rLen; r++)     {
            int a = r * std::cos(gradient_dir.at&lt;double&gt;(y, x));
            int b = r * std::sin(gradient_dir.at&lt;double&gt;(y, x));
            int x0 = x - a;
            int y0 = y - b;
            if(x0 &gt;= 0 &amp;&amp; x0 &lt; image.cols &amp;&amp; y0 &gt;= 0 &amp;&amp; y0 &lt; image.rows )  {
                houghSpace.at&lt;cv::Vec3i&gt;(y0,x0)[r]++;
            }
            x0 = x + a;
            y0 = y + b;
            if(x0 &gt;= 0 &amp;&amp; x0 &lt; image.cols &amp;&amp; y0 &gt;= 0 &amp;&amp; y0 &lt; image.rows )  {
                houghSpace.at&lt;cv::Vec3i&gt;(y0,x0)[r]++;
            }
        }
    }
}


In C++11, I'd use a range-for.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.

Use const where possible

The HoughTransformCircles routine does not  alter the image passed to it, and so it should be declared as taking const Mat&amp; image as an argument.