Why isn't our c# graphics code working any more?

Question

Here's the situation:

We have some generic graphics code that we use for one of our projects. After doing some clean-up of the code, it seems like something isn't working anymore (The graphics output looks completely wrong).

I ran a diff against the last version of the code that gave the correct output, and it looks like we changed one of our functions as follows:

static public Rectangle FitRectangleOld(Rectangle rect, Size targetSize)
{
    if (rect.Width <= 0 || rect.Height <= 0)
    {
        rect.Width = targetSize.Width;
        rect.Height = targetSize.Height;
    }
    else if (targetSize.Width * rect.Height > 
        rect.Width * targetSize.Height)
    {
        rect.Width = rect.Width * targetSize.Height / rect.Height;
        rect.Height = targetSize.Height;
    }
    else
    {
        rect.Height = rect.Height * targetSize.Width / rect.Width;
        rect.Width = targetSize.Width;
    }

    return rect;
}

to

static public Rectangle FitRectangle(Rectangle rect, Size targetSize)
{
    if (rect.Width <= 0 || rect.Height <= 0)
    {
        rect.Width = targetSize.Width;
        rect.Height = targetSize.Height;
    }
    else if (targetSize.Width * rect.Height > 
             rect.Width * targetSize.Height)
    {
        rect.Width *= targetSize.Height / rect.Height;
        rect.Height = targetSize.Height;
    }
    else
    {
        rect.Height *= targetSize.Width / rect.Width;
        rect.Width = targetSize.Width;
    }

    return rect;
}

All of our unit tests are all passing, and nothing in the code has changed except for some syntactic shortcuts. But like I said, the output is wrong. We'll probably just revert back to the old code, but I'm curious if anyone has any idea what's going on here.

Thanks.

Answer 1

Sounds like you don't have sufficient unit tests :]

Unfortunately, your statement

"Nothing in the code has changed except for some syntactic shortcuts"

is wrong, and I'm guessing that's where your problem is. (It's certainly one of your problems!)

Yes,

a *= b;

is equivalent to

a = a * b;

but

a *= b / c;

is NOT the same as

a = a * b / c;

instead

a *= b / c;    // equivalent to a = a * (b / c)
a = a * b / c; // equivalent to a = (a * b) / c

(See c# operator precedence on msdn)

I'm guessing you're running into trouble when your target height is not an exact multiple of the original rectangle height (or the same for the width).

Then you'd end up with the following sort of situation:

Let's assume rect.Size = (8, 20), targetSize = (15, 25)

Using your original method, you'd arrive at the following calculation:

rect.Width     = rect.Width * targetSize.Height / rect.Height;
//             = 8          * 25                / 20
//             = 200 / 20 (multiplication happens first)
//             = 10
// rect.Width  = 10

Using your new code, you'd have

rect.Width    *= targetSize.Height / rect.Height;
//            *= 25 / 20
//            *= 1 (it's integer division!)
// rect.Width  = rect.Width * 1
//             = 8
// rect.Width  = 8

which isn't the same. (It get's worse if the target size is less than your original size; in this case the integer division will result in one of the dimensions being 0!)

If "[your] unit tests are all passing" then you definitely need some additional tests, specifically ones that deal with non-integer multiples.

Also note that your calculation

else if(targetSize.Width * rect.Height > 
        rect.Width * targetSize.Height)

isn't reliable; for very large rectangles, it has the potential to overflow and give you incorrect results. You'd be better off casting to a larger type (i.e. a long) as part of the multiplication. (Again, there should be some unit tests to this effect)

Hope that helps!

Answer 2

If Rectangle.Width and Rectangle.Height are integers, the following two lines differ:

rect.Width = rect.Width * targetSize.Height / rect.Height;
rect.Width *= targetSize.Height / rect.Height;

The first line performs a multiply, divide, cast-to-int, then assignment, in that order. The second performs a divide, cast-to-int, multiply, then assignment. The problem is, in your non-working code, your divide is being casted to an integer before the multiply.

Keep the original code or force the division to be floating-point.

Write better unit tests to check for this issue. (Try a width/height combination which do not have even multiples (e.g. prime numbers).)

Answer 3

To add to Daniel L's answer.

What is the point of this 'optimization'? There are better ways to clean up this code, and make it more readable.