Checking to see if an area of the screen changes

I'm testing some methods for checking if an area of the screen changes, can you see any problems with how I'm doing this, and is there a better way for me to do this?

the _memoryImage2 will not change in the final app, it will be a read only array that I will check the memoryImage against.

```
namespace ScreenShot_BoundsTest
{
public partial class Form1 : Form
{
Bitmap _memoryImage1;
Bitmap _memoryImage2;

Color[,] _first;
Color[,] _second;

private string _valueX;

public Form1()
{
InitializeComponent();
}

private void Button1Click(object sender, EventArgs e)
{
backgroundWorker1.RunWorkerAsync();
}

private void GetImage()
{
_memoryImage2 = _memoryImage1;
CaptureScreen();
pictureBox2.Image = pictureBox1.Image;
if (_memoryImage1 != null) _first = Bitmap2Imagearray(_memoryImage1);

pictureBox1.Image = _memoryImage1;
if (_memoryImage2 != null) _second = Bitmap2Imagearray(_memoryImage2);

var x = CompareArrays();

_valueX = x != true ? "false" : "true";
}

private bool CompareArrays()
{
for (var i = 0; i < _memoryImage1.Width; i++)
{
for (var j = 0; j < _memoryImage1.Height; j++)
{
if (_first == null) continue;
if (_second != null)
if (_first[i,j] != _second[i,j])
{
return false;
}
}
}
return _first != null && _second != null;
}

private void CaptureScreen()
{
Size s;
using (var myGraphics = CreateGraphics())
{
s = new Size(100, 50);
_memoryImage1 = new Bitmap(s.Width, s.Height, myGraphics

Let's start with your algorithm. You're using Bitmap.GetPixel to convert the entire bitmap image pixel by pixel into matrix of colors. I tried this approach several years ago - it was pretty slow on .Net 2.0 and I suppose it is still slow. You should convert image into byte[] and extract color bytes out of this array, it should be hundred times faster. Look into Bitmap.Lockbits method and BitmapData class.

Remaining points are about your code readability. There are several general rules which may improve readability a lot:

• If method has some result of execution then return this result instead of assigning it to some field. Also use method parameters to accept arguments instead of class fields.

• If variable is not needed outside of one method's scope then do not declare a field for it, use local variable instead.

• Be strongly typed. Do not cast anything to string until you really need string.

These simple rules being applied to your code mean:

-
Rules #1 and #3 mean that your methods should have following signature:

• void GetImage() -> bool CompareCurrentImageWithPrevious()

• bool CompareArrays() -> static bool CompareImageArrays(Color[,] imageArray1, Color[,] imageArray2)

• void CaptureScreen() -> Bitmap CaptureScreen()

-
This

var x = CompareArrays();
_valueX = x != true ? "false" : "true";

Should be replaced with:

return CompareArrays();

-
This

for (var i = 0; i < _memoryImage1.Width; i++)
{
    for (var j = 0; j < _memoryImage1.Height; j++)
    {
        if (_first == null) continue;
        if (_second != null)
            if (_first[i,j] != _second[i,j])
            {
                return false;
            }
    }
}
return _first != null && _second != null;

Should be:

if (_first == null || _second == null) return false;
 if (_memoryImage1.Width != _memoryImage2.Width ||
     _memoryImage1.Height != _memoryImage2.Height)
     return false;
 return _first.SequenceEquals(_second);

-
Do not assign result of GetImage() to class field, DoWorkEventArgs have Result property which can be used to return value computed by background worker.

private void BackgroundWorker1DoWork(object sender, DoWorkEventArgs e)
{
    e.Result = GetImage();
    Thread.Sleep(100);
}

StackExchange Code Review Q#3053, answer score: 8