Histogram of a RGB image using OpenCV

I wrote some C++ code using the OpenCV library. The program reads the image and plot histograms of Red, Green, and Blue components. I am very new to OpenCV so I need comments and advice. What do you think about my code? Is there any better way to do that? Are there unnecessary parts in my code? Is there any part I should change?

#include 
#include 
#include 
#include 
using namespace std;
using namespace cv;
int main(){
    Mat image = imread("C:/image.jpg");
    int HistR[257] = {0};
    int HistG[257] = {0};
    int HistB[257] = {0};
    for (int i = 0; i (Point(j, i));
            int Red = intensity.val[0];
            int Green = intensity.val[1];
            int Blue = intensity.val[2];
            HistR[Red] = HistR[Red]+1;
            HistB[Blue] = HistB[Blue]+1;
            HistG[Green] = HistG[Green]+1;
        }
    Mat HistPlotR (500, 256, CV_8UC3, Scalar(0, 0, 0));
    Mat HistPlotG (500, 256, CV_8UC3, Scalar(0, 0, 0));
    Mat HistPlotB (500, 256, CV_8UC3, Scalar(0, 0, 0));
    for (int i = 0; i < 256; i=i+2)
    {
        line(HistPlotR, Point(i, 500), Point(i, 500-HistR[i]), Scalar(0, 0, 255),1,8,0);
        line(HistPlotG, Point(i, 500), Point(i, 500-HistG[i]), Scalar(0, 255, 0),1,8,0);
        line(HistPlotB, Point(i, 500), Point(i, 500-HistB[i]), Scalar(255, 0, 0),1,8,0);
    }
    namedWindow("Red Histogram");
    namedWindow("Green Histogram");
    namedWindow("Blue Histogram");
    imshow("Red Histogram", HistPlotR);
    imshow("Green Histogram", HistPlotG);
    imshow("Blue Histogram", HistPlotB);
    waitKey(0);
    return 0;
}

Overall, this is pretty straightforward, easy-to-read code. Nicely done.

As @Loufylouf mentioned in the comments, there is a function in OpenCV that calculates a histogram already - the calcHist function. You'd be better off using that. But it doesn't hurt to try to do it yourself just to see how it's done.

You're calculating the histogram of an image with 8-bit per channel RGB pixels. You only need 256 bins, not 257. You're allocating an extra int for each histogram. That's not a huge problem, but you don't need it. (Unlike C strings, arrays do not need a terminating value or anything like that.)

Also, there's a lot of magic numbers in the code. Instead of:

int HistR[256] = { 0 };

You could tell a potential future reader of your code what the 256 (or 257) is. Something like:

const int MAX_BINS = 256;
...
int HistR[MAX_BINS] = { 0 };

Or, since the reason you have 256 bins is because the values you're analyzing are unsigned 8-bit characters, you could use the predefined UINT8_MAX constant. (It's in stdint.h.) Just remember to add 1 to it.

If it were my code, I might even define constants for red, green, and blue you use with the Vec3b type:

const int RED_CHAN = 0;
const int GREEN_CHAN = 1;
const int BLUE_CHAN = 2;

and then:

int Red = intensity.val[RED_CHAN];
        int Green = intensity.val[GREEN_CHAN];
        int Blue = intensity.val[BLUE_CHAN];

It just clarifies what you're pulling out of the array.

Or, since this is C++, you could subclass Vec3b to add a red(), green() and blue() method and then just write:

int Red = intensity.red();

And I'd also make a constant to use instead of the 500 you have for the width of the histogram images.

I'd break this up into 3 functions - calculateHistogram(), drawHistogram(), and showImage(). calculateHistogram() would have the double loop where you add up values in the different bins. drawHistogram() would generate the HistPlot* images, and showImage() would create the windows and show the images.

Lastly, I'd add some white space! A blank line here or there to group together related lines helps me mentally organize what's going on.

StackExchange Code Review Q#94434, answer score: 3