Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to optimize the following code:

    private void Screen_Paint(object sender, PaintEventArgs e)
    {
        Graphics bmpGraphics = Graphics.FromImage(_screenBitmap);

        int xLoc = 0;
        int yLoc = 0;

        bmpGraphics.Clear(Color.Black);
        _resetInvoked = true;

        for (int i = 0; i < 4000; i += 2)
        {
            var bgBit = (byte) ((_screenMemory[i + 1]) & 112);
            var fgBit = (byte)((_screenMemory[i + 1]) & 15);
            SolidBrush bgBrush = _backgroundBrushesArray[bgBit/16];
            SolidBrush fgBrush = _foregroundBrushesArray[fgBit];

            if (((xLoc % 640) == 0) && (xLoc != 0))
            {
                yLoc += 12;
                xLoc = 0;
            }
            if (_resetInvoked)
            {
                string s = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
                var pf = new PointF(xLoc, yLoc);

                bmpGraphics.FillRectangle(bgBrush, xLoc + 2, yLoc + 3, 8f, 11f);
                bmpGraphics.DrawString(s, _renderFont, fgBrush, pf);

            }
            xLoc += 8;
        }

        _resetInvoked = false;

        e.Graphics.DrawImage(_screenBitmap, new Point(0, 0));
        bmpGraphics.Dispose();
    }

This is a Winforms User Control that mimics a console like screen. Its working just fine, but I'd like to speed it up. Running it through Telerik's JustCode performance monitor, the slowest performance seems to come from the FillRectangle and DrawString methods. Is there anyway to speed this up?

share|improve this question
1  
Why do you set _resetInvoked to true in the beginning and then set to false, and why is if (_resetInvoked) in there if it's always true? – janos Dec 1 '14 at 20:11
    
@janos -- I only copied in the relevant code. But I have it in there because my application is multithreaded and I _resetInvoked is a flag used to determine, basicially, if the Paint event is currently already "painting". I didn't want the Paint event to execute if there is already a painting going on. – Icemanind Dec 1 '14 at 21:20
    
_resetInvoked is a strange name for that, unless it is only set during reset events. I would recommend something more like _busy since that's what you seem to be using it for. – Nick Udell Dec 2 '14 at 9:44

1 Answer 1

up vote 3 down vote accepted

Magic numbers

You have a lot of magic numbers in your code. Consider to express them as meaningful constants. ( 4000, 112, 15, 16, 640, 12, 8f, 11f)

Style

Be consistent with your coding style.

string s = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
var pf = new PointF(xLoc, yLoc);  

at the first line you explicitly declare s as string also it is quite obvious by the right hand side that it is a string. On the second line you use var as expected.

There is nothing wrong with declaring implict by using the type, but here you should have either used var also or instead of var PointF.

Naming

Use meaningful names for variables. Mr.Maintainer will thank you for doing so. s and pf are poor names.

Using statement

Instead of creating and disposing the Graphics you should enclose this inside a using statement, so in case of an exception the Graphics object is properly disposed.

Declaring variables

Variables should be declared as nearly as possible of their use ( here: the brushes ).

Refactoring

This won't do anything regarding speed, if the bottleneck are the FillRectangle() and DrawString() methods.

There is no sense in entering the loop or doing anything if _resetInvoked==true. So using a guard condition to return early is the way to go.

private void Screen_Paint(object sender, PaintEventArgs e)
{

    if (_resetInvoked) { return; }  
    _resetInvoked = true;

    using (Graphics bmpGraphics = Graphics.FromImage(_screenBitmap))
    {

        int xLoc = 0;
        int yLoc = 0;

        bmpGraphics.Clear(Color.Black);

        for (int i = 0; i < 4000; i += 2)
        {

            if (((xLoc % 640) == 0) && (xLoc != 0))
            {
                yLoc += 12;
                xLoc = 0;
            }

            var bgBit = (byte) ((_screenMemory[i + 1]) & 112);
            SolidBrush bgBrush = _backgroundBrushesArray[bgBit/16];
            bmpGraphics.FillRectangle(bgBrush, xLoc + 2, yLoc + 3, 8f, 11f);

            var fgBit = (byte)((_screenMemory[i + 1]) & 15);
            SolidBrush fgBrush = _foregroundBrushesArray[fgBit];

            var currentText = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
            var currentPoint = new PointF(xLoc, yLoc);
            bmpGraphics.DrawString(currentText, _renderFont, fgBrush, currentPoint);

            xLoc += 8;
        }

    }

    e.Graphics.DrawImage(_screenBitmap, new Point(0, 0));
    _resetInvoked = false;
}
share|improve this answer
    
The default constructor for Point will create you a point at 0,0. There is no need to specify it. Otherwise, great answer as usual. – d347hm4n Dec 2 '14 at 10:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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