6
\$\begingroup\$

This draws scanline of the image to System.Drawing.Graphics. It's quite simple and it's optimized in order to merge neigbour samples with the same color into single rectangle (instead of drawing sample-by-sample in 1x1 rectangles).

I don't like the duplication of this code inside and outside the loop:

updateAlpha(brush, previousColor);
graphics.FillRectangle(brush, previousIndex + positionX, y, i - previousIndex, 1);

Do you know the good way how to follow DRY principle here and to keep code easy to understand? Or this is a kind of perfectionism and all is already fine?

private static void drawGlyphRowToGraphics(int rowIndex, FT_Bitmap glyphBitmap, 
    Graphics graphics, float positionX, float positionY, SolidBrush brush)
{
    float y = rowIndex + positionY;

    byte previousColor = 0;
    int previousIndex = 0;
    for (int i = 0; i < glyphBitmap.Width; ++i)
    {
        byte color = (byte)(255 - glyphBitmap.Buffer[i + rowIndex * glyphBitmap.Width]);
        if (i == 0)
            previousColor = color;

        if (color != previousColor)
        {
            updateAlpha(brush, previousColor);
            graphics.FillRectangle(brush, previousIndex + positionX, y, i - previousIndex, 1);

            previousColor = color;
            previousIndex = i;
        }
    }

    if (glyphBitmap.Width > 0)
    {
        updateAlpha(brush, previousColor);
        graphics.FillRectangle(brush, previousIndex + positionX, y, glyphBitmap.Width - previousIndex, 1);
    }
}

private static void updateAlpha(SolidBrush brush, byte gray)
{
    brush.Color = Color.FromArgb(255 - gray, brush.Color.R, brush.Color.G, brush.Color.B);
}
\$\endgroup\$
  • \$\begingroup\$ My first thought is to pull out updateAlpha and your FillRectangle methods into a method with a single parameter that would replace (i|glyphBitmap.Width) Then you could just call that single method. Granted it is still twice, but without putting your code into a test of mine I can't determine if I would break anything. Maybe you could consider using a do/while loop. \$\endgroup\$ – Robert Snyder Sep 18 '13 at 2:43
  • 1
    \$\begingroup\$ Simple extraction of common method for updateAlpha and FillRectangle was the first thought, but such method doesn't look well-designed. We should pass many different arguments there and also it has not single clean goal. So I'd rather prefer to refactor method to avoid calling these methods in two places. \$\endgroup\$ – Vitaliy Shibaev Sep 19 '13 at 0:27
6
\$\begingroup\$

First off I have to say I found this code very hard to understand. It took me quite some time to finally understand what this code actually does.

First off the function takes 6 arguments. That's way too many. Clearly the positionX and positionY should be combined into one Point object.

But the larger problem is, that you seem to be working at the wrong level of abstraction. The function is supposed to deal with one single row of a FT_Bitmap, but because there is no abstraction of such a single row, it ends up dealing with the whole FT_Bitmap, doing confusing offset calculations all over the place.

What I would do first, is to create a separate GlyphRow class:

public class GlyphRow
{
    private FT_Bitmap bitmap;
    private int offset;

    public int Width;

    public GlyphRow(FT_Bitmap bitmap, int rowIndex)
    {
        this.bitmap = bitmap;
        this.offset = rowIndex * bitmap.Width;
        this.Width = bitmap.Width;
    }

    // Returns the transparency value of a point in position x of our row
    public byte Alpha(x)
    {
        return bitmap.Buffer[offset + x];
    }

}

This will allow us to work with a single row at a time.

Now our function can take just the following arguments:

private static void drawGlyphRowToGraphics(
    GlyphRow row, Graphics graphics, Point position, SolidBrush brush)

And when calling it, we initialize Point and GlyphRow objects:

GlyphRow row = new GlyphRow(glyphBitmap, rowIndex);
Point position = new Point(positionX, rowIndex + positionY);
drawGlyphRowToGraphics(row, graphics, position, brush);

Note that the position is now exactly the position where this GlyphRow should be painted, already offset with rowIndex.

So here's the new simplified drawGlyphRowToGraphics:

private static void drawGlyphRowToGraphics(
    GlyphRow row, Graphics graphics, Point position, SolidBrush brush)
{
    int i = 0;
    while (i < row.Width)
    {
        int length = sameAlphaLength(row, i);
        brush.Color = cloneColorWithAlpha(brush.Color, row.Alpha(i));
        graphics.FillRectangle(brush, position.X + i, position.Y, length, 1);
        i += length;
    }
}

I found the algorithm in original implementation very hard to grasp, so using divide-and-conquer strategy, I created a separate function to calculate the length of a section that has the same transparency value:

private static int sameAlphaLength(GlyphRow row, int start)
{
    byte alpha = row.Alpha(start);
    int length = 1;
    while (start + length < row.Width)
    {
        if (alpha != row.Alpha(start + length))
        {
            return length;
        }
        length++;
    }
    return length;
}

In the original code, at first the gray value was calculated with (255 - alpha), and then passed to updateAlpha, where it was again transformed with (255 - gray). Pretty redundant IMHO. So I'm skipping this whole double-conversion. I also find it better to work with functions without side-effects, so I replaced updateAlpha with a function that creates a new color value from an existing one, which I can then assign to brush:

private static Color cloneColorWithAlpha(Color color, byte alpha)
{
    return Color.FromArgb(alpha, color.R, color.G, color.B);
}

Disclaimer: I've never written a single line of C# before. It probably doesn't compile, not to mention other possible bugs. But hoping to get the general point across.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for (working at the wrong level of) abstraction and gathering all the bits into a GlyphRow abstraction. Abstracting fundamental problem-domain stuff seems to naturally engender object oriented goodness throughout the rest of the program. \$\endgroup\$ – radarbob Sep 18 '13 at 16:01
  • \$\begingroup\$ Thank you for perfect refactoring example. Wrong level of abstraction here is a very good point. I will check this suggestion, but in general it looks correct and should improve code clarity. \$\endgroup\$ – Vitaliy Shibaev Sep 19 '13 at 0:22

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.