4
\$\begingroup\$

I have code that loading bitmap from file. And searching for black rectangle(5*5). Can you help me to review it?

namespace TakeScreen
{
    class Program
    {
        static void Main(string[] args)
        {
            List<List<Color>> Sequence = new List<List<Color>>();

            for (int i = 0; i < 5; i++)
            {
                List<Color> vector = new List<Color>();
                for (int j = 0; j < 5; j++)
                {
                    vector.Add(Color.Black);
                }
                Sequence.Add(vector);
            }

            TakeScreen TS = new TakeScreen();
            Bitmap bitmap = TS.LoadImage(@"D:\ScreenShot.png");
            List<Point> points = TS.FindPixelSequence(bitmap, Sequence);
        }
    }
    public class TakeScreen
    {
        public Bitmap MakeScreenShot()
        {
            Bitmap bmpScreenCapture = new Bitmap(Screen.PrimaryScreen.Bounds.Width, Screen.PrimaryScreen.Bounds.Height);
            using (Graphics g = Graphics.FromImage(bmpScreenCapture))
            {
                g.CopyFromScreen(Screen.PrimaryScreen.Bounds.X,
                                 Screen.PrimaryScreen.Bounds.Y,
                                 0, 0,
                                 bmpScreenCapture.Size,
                                 CopyPixelOperation.SourceCopy);
            }
            return bmpScreenCapture;
        }

        public Bitmap LoadImage(string filePath)
        {
            Bitmap bmpScreenCapture = new Bitmap(filePath);
            return bmpScreenCapture;
        }

        public void SaveScreenShot(Bitmap bitmap, string filePath)
        {
            bitmap.Save(filePath);
        }
        public List<Point> FindPixelSequence(Bitmap bitmap, List<List<Color>> soughForRectangele)
        {
            List<Point> Points = new List<Point>();

            byte r0;
            byte g0;
            byte b0;

            List<List<Color>> Rectangle = new List<List<Color>>();
            List<Color> rectangleRow;

            Stopwatch watch = new Stopwatch();
            watch.Start();
            BitmapData data = bitmap.LockBits(new System.Drawing.Rectangle(0, 0, bitmap.Width, bitmap.Height), ImageLockMode.ReadOnly, PixelFormat.Format32bppArgb);
            unsafe
            {
                byte* ptrSrc = (byte*)data.Scan0;
                for (int y = 0; y < data.Height; y = y + 1)
                {
                    for (int x = 0; x < data.Width; x = x + 1)
                    {
                        Rectangle = new List<List<Color>>();
                        r0 = ptrSrc[2];
                        g0 = ptrSrc[1];
                        b0 = ptrSrc[0];
                        Color currentPixel = Color.FromArgb(0, r0, g0, b0);
                        if (currentPixel.R == soughForRectangele[0][0].R && currentPixel.G == soughForRectangele[0][0].G && currentPixel.B == soughForRectangele[0][0].B)
                        {
                            for (int j = 0; j < 5; j++)
                            {
                                rectangleRow = new List<Color>();
                                for (int i = 0; i < 5; i++)
                                {
                                    r0 = ptrSrc[2];
                                    g0 = ptrSrc[1];
                                    b0 = ptrSrc[0];
                                    rectangleRow.Add(Color.FromArgb(255, r0, g0, b0));
                                    ptrSrc += 4;
                                }
                                ptrSrc -= (4 * 5);
                                Rectangle.Add(rectangleRow);

                                ptrSrc += (4 * data.Width);
                            }
                            ptrSrc -= (4 * data.Width) * 5;

                            if (CompareRectangels(soughForRectangele, Rectangle))
                            {
                                Points.Add(new Point { X = x, Y = y });
                            }
                        }
                        ptrSrc += 4;
                    }
                }
            }
            bitmap.UnlockBits(data);
            watch.Stop();
            Console.WriteLine(watch.ElapsedMilliseconds);
            return Points;
        }

        public bool CompareRectangels(List<List<Color>> soughForRectangele, List<List<Color>> currentRectangele)
        {
            ClassqualityComparer C = new ClassqualityComparer();
            List<bool> results = new List<bool>();
            for (int i = 0; i < 5; i++)
            {
                results.Add(C.Equals(soughForRectangele[i], currentRectangele[i]));
            }
            if (results.Contains(false))
            {
                return false;
            }
            else
            {
                return true;
            }
        }
    }
    public class ClassqualityComparer : IEqualityComparer<List<Color>>
    {
        public bool Equals(List<Color> x, List<Color> y)
        {
            for (int i = 0; i < 5; i++)
            {
                if (x[i].R != y[i].R && x[i].G != y[i].G && x[i].B != y[i].B) return false;
            }
            return true;
        }

        public int GetHashCode(List<Color> obj)
        {
            throw new NotImplementedException();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ are you also working on kind of screen sharing project? \$\endgroup\$ – Slashy Aug 5 '16 at 8:48
3
\$\begingroup\$

Just a few remarks ...

1. Use "var" where it is possible. Code will be more readable. For example here

List<List<Color>> Rectangle = new List<List<Color>>();
  1. Single responsibility principle is violated. Your class takes screenshot, load images, finds sequences ...

3.Rename "Sequence" to "blackRectangle" because it's quite difficult to understand from your code what you are doing. i understand it only because you wrote about black rectangle

4.On the first sight your outer loops can be cut off.

(int y = 0; y < data.Height)

I suppose this pseudo-code would be better

y<data.Height-soughForRectangele.Height 
\$\endgroup\$
1
\$\begingroup\$

be consistent with your naming standards. You will be much happier when you revisit your code in 6 months time

Do not name local variables with names starting with Cap letter (Rectangle, Points,...) those names are for Classes, Functions and Properties

Dont use 'hungarian' or hungarian-like notation. bmpScreenCapture, just call it screenBitmap.

Name lengths: v short or single letters are for loop indexes or very tight scopes. You have variables called 'C', 'TC', 'g'. (Note the inconsistent case here !)

Dont use unhelpful names like vector and Sequence (case again !), vector of what, Sequence of what

\$\endgroup\$

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.