Color guessing code

This code is supposed to learn colors from many many data images, and then recognize color using an algorithm that I made. Eventually, I want the program to make its own algorithm.

Data input and test cases are on GitHub.

And now, the actual code:

```
// By Dat HA

#include "stdafx.h" // visual studio mandatory include file

#include // open cv libraries
#include
#include
#include

#include // windows library

#include // std libraries
#include
#include
#include
#include

const int NUM_FILE = 10; // number of images per color, NUM_FILE & NUM_COLOR are temporary until I get Boost working
const int NUM_COLOR = 7; // number of colors
const float NUM_VERSION = 1.3; // version number
const std::string TRAIN_DATA_FOLDER = "../TrainData/"; // training data location

struct Color { // define a new color that we learned of compare
std::string colorName; // name of the color, ex. red, blue
cv::Scalar bgr; // blue, green, and red values in that order
cv::Scalar difference; // what is the difference between, blue and green, green and red, and red and blue
};

cv::Scalar getAvg(std::vector imgData) { // get the average BGR of a vector of images BGR value
cv::Scalar avg = { 0,0,0,0 }; // new scalar
int num = imgData.size(); // size of vector
for (int rgb = 0; rgb &color) { // train the neural network
for (int j = 0; j > colorName; // get the string frmo text file to color name variable
file.close(); // close text file
std::vector imgData; // vector of image BGRs in a scalar format
for (int i = 0; i color, cv::Mat image) { // guest the color
cv::Scalar imgBgr = cv::mean(image); // get average BGR value of image
cv::Scalar imgDifference = getBgrDifference(imgBgr); // get BGR's difference
std::vector accuracy; // create a vector for accuracy, higher the better

for (int i = 0; i color; // color vector
training(color); // train neural net and store learned color in vector

// TESTTIN

Fix the includes

Don't include headers you're not using. I had to remove a bunch of headers that aren't available here, simply to get your code to compile.

I then had to include the math header for the use of fabs in getColorAccuracy() - which I then changed to std::abs, so `.

I then had:

#include 
#include 

#include 
#include 
#include 
#include 
#include 

Comments should explain the code

Throughout the program, your comments duplicate what the code says. Good comments explain why rather than what, and most of the commentary is through good use of names. For example:

cv::Scalar getAvg(std::vector imgData) { // get the average BGR of a vector of images BGR value
    cv::Scalar avg = { 0,0,0,0 }; // new scalar
    int num = imgData.size(); // size of vector
    for (int rgb = 0; rgb < 3; rgb++) { // cycle through the colors
        for (int i = 0; i < num; i++) // cycle through the pictures
            avg[rgb] += imgData[i][rgb]; // add them up
        avg[rgb] /= num; // divide them by the total
    }
    return avg; // return the average
}

We know that imgData.size() returns the size of a vector, so no need to comment that. It's much more important, for example, to explain why we loop over the RGB components in the outer loop rather than in the inner loop.

Use range-based for on containers

In getAvg(), we don't need to count elements if we use range-based for (and this eliminates a dubious conversion in int num = imgData.size()):

cv::Scalar getAvg(const std::vector& imgData)
{
    cv::Scalar avg{ 0 };
    for (auto const& img: imgData) {
        avg += img;
    }

    double const n = imgData.size();
    return avg / n;
}

Here, I've also made use of the operator overloads += and / provided by cv::Scalar to perform elementwise arithmetic without needing to loop over the RGBA components within the Scalar.

Note also that this function can accept the vector by const reference, as we are not modifying it and do not need to copy.

Create and open ifstream in a single step

Instead of creating a default-constructed stream, we can start with it open, by passing the filename to its constructor:

std::string colorName;
    {
        std::ifstream file{TRAIN_DATA_FOLDER + std::to_string(j) + "/name.txt"};
        file >> colorName;
    }

It's not necessary to explicitly call file.close(), as the destructor takes care of that for us.

Reduce copying of Color objects

We can use std::vector::emplace_back to construct a Color directly into the vector:

color.emplace_back(colorName,
                       getAvg(imgData),
                       getBgrDifference(getAvg(imgData)));

I'll change the Color constructor to accept two arguments and compute the difference, which will eliminate the second call to getAvg() - see the complete code at the end of answer.

A simple typo

I think that Guest should be Guess!

Inefficient search

The getColorGuess() is the only function that uses C++ algorithms, but this line looks quite dubious:

Color bestColor = color[std::distance(accuracy.begin(),
                                      std::find(accuracy.begin(),
                                                accuracy.end(), 
                                                *std::max_element(accuracy.begin(), accuracy.end())))];

Having found an iterator to the maximum value, there's no need to dereference it and pass it to find to get the same iterator back again. It's functionally equivalent to

Color bestColor = color[std::distance(accuracy.begin(),
                                      std::max_element(accuracy.begin(), accuracy.end()))];

We can do still better, though, as we can find the maximum value directly on the color vector, by telling std::max_element how to do the calculation:

auto it = std::max_element(color.begin(),
                           color.end(),
                           [imgDifference](const Color& a, const Color& b) {
                               return getColorAccuracy(imgDifference, a.difference) < getColorAccuracy(imgDifference, b.difference);
                           });
// *it is a reference to a const Color in the vector

Busy loop

This is rude to any other process (and to those of us who prefer a cooler environment):

while (1);

This never terminates. Just remove it.

Re-worked code

``
#include
#include

#include
#include
#include
#include
#include
#include

const int NUM_FILE = 10; // number of images per color, NUM_FILE & NUM_COLOR are temporary until I get Boost working
const int NUM_COLOR = 7; // number of colors
const float NUM_VERSION = 1.3; // version number
const std::string TRAIN_DATA_FOLDER = "../TrainData/"; // training data location

cv::Scalar getBgrDifference(const cv::Scalar& bgr);

struct Color {
std::string colorName;
cv::Scalar bgr;
cv::Scalar difference;
Color(std::string, cv::Scalar bgr)
: colorName{

StackExchange Code Review Q#162913, answer score: 8