8
\$\begingroup\$

I need to be able to detect if an image has the colour Violet in it. (Follow-up)

The problem that I'm facing is that there are quite a number of shades of violet that it seems almost impossible it add every single one.

 private void ProcessImage(PictureBox picture)
    {
        using (System.IO.MemoryStream ms = new System.IO.MemoryStream())
        {
            List<Color> ColoursToDetect = new List<Color>()
            {
                Color.FromArgb(202,156,254),
                Color.FromArgb(143,125,151),
                Color.FromArgb(100,76,136),
                Color.FromArgb(232,175,254)
            };
            Boolean colour_Found = false;
            Bitmap SelectedImage = new Bitmap(picture.Image);
            Color selected_Pixel;

            for (int x = 0; x != SelectedImage.Width; x++)
            {
                for (int y = 0; y != SelectedImage.Height; y++)
                {
                    selected_Pixel = SelectedImage.GetPixel(x, y);
                    foreach (Color c in ColoursToDetect)
                    {
                        if (c == selected_Pixel)
                        {
                            colour_Found = true;
                            MessageBox.Show("Found");
                            colour_Found = true;
                        }
                    }
                    if (colour_Found)
                    {
                        break;
                    }
                }
            }
        }
    }

The above code is working (So to speak) However I'm worried that one day an image will have a certain violet that i have no added and in hindsight will make the code useless.

I did look at the .Net library AForge http://www.aforgenet.com/projects/iplab/, However I couldn't find any information about detecting a range of a certain colour.

\$\endgroup\$
  • 2
    \$\begingroup\$ It seems to me that your solution depends heavily on a working definition of "shade of violet", and that means either listing a huge number of them individually, or listing a smaller set of ranges, or a range of proportions between R,G, and B. Without knowing what constitutes "violet", how does your method know what to find? \$\endgroup\$ – Rick Davin Jun 7 '18 at 12:44
  • 3
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mathias Ettinger Jun 7 '18 at 14:01
17
\$\begingroup\$

Try using HSV/HSL to represent color instead of RGB.

You can then define violet as some hue range, e.g. 250° to 310° and similarly for saturation and lightness/value.


Always use a using when you have tempoary disposable resources like a Bitmap


Variables in C# are camelCase:

Boolean colourFound = false;
Bitmap selectedImage = new Bitmap(picture.Image);
Color selectedPixel;

Try to limit the scope of variables, move the declaration of selectedPixel to where you currently assign it.

\$\endgroup\$
8
\$\begingroup\$

var keyword

The var keyword makes things like using (System.IO.MemoryStream ms = new System.IO.MemoryStream()) into using (var ms = new System.IO.MemoryStream()) which is much nicer, don't you agree? Plus there's no change to the runtime performance of the code, since the compiler just infers what the type is based on its assignment.

Exact Conditions for Loops

I would be hesitant to have an exact condition such as x != selectedImage.Width since there's always a chance for weird stuff to happen (whether it be a bug now or one introduced later). Maybe someone decides to set the value of x inside the for-loop at a later date and manages to set it higher than the width and now you've got yourself an infinite loop. Something like x < selectedImage.Width avoids this minor potential (why take risks that are unnecessary, not matter how small?).

Definition of Violet

Just like Rick Davin commented, you can't exactly properly test for violet if you haven't fully defined what violet is. You're probably going to need to do some research to find out the best way for your program to determine what violet is and then implement this method. Nevertheless, you'll need a strong definition of what violet is before you can test for it.

\$\endgroup\$
4
\$\begingroup\$

This should not return void. It should not be writing to the UI.

private bool ProcessImage(PictureBox picture)

You don't use ms.

Why do you keep looping after a match? Why set colour_Found = true; twice?

foreach (Color c in ColoursToDetect)
{
    if (c == selected_Pixel)
    {
        return true;
    }
}

for (int x = 0; x < SelectedImage.Width; x++)
{
}
return false;

Use of case is not consistent: SelectedImage colour_Found.

Free up memory

\$\endgroup\$
  • \$\begingroup\$ Thanks for your response, I more so trying to figure out if there is a way it can detect all shades of purple/violet, \$\endgroup\$ – user1234433222 Jun 7 '18 at 11:50
  • \$\begingroup\$ @AndrewEberle This is code review. \$\endgroup\$ – paparazzo Jun 7 '18 at 11:51
  • \$\begingroup\$ I understand, but because the code is "technically" working, i thought this would be a better place to post. \$\endgroup\$ – user1234433222 Jun 7 '18 at 11:52
  • 3
    \$\begingroup\$ You posted on code review and you got a code review. \$\endgroup\$ – paparazzo Jun 7 '18 at 11:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.