4
\$\begingroup\$

I am working with a system that needs to consume product images uploaded by employees and resize them / add white padding to the sides to create square images to be consumed by our e-commerce site. I'm attempting to keep things somewhat light on the memory usage because this will live in a windows service on an azure vm. I know there are better solutions, but that's what I'm given as a platform. I also don't want to use 3rd party libraries.

The following code is what I have so far. Please, pick it apart and let me know if i'm making any mistakes.

public static Image ImageToFixedSize(Image originalImage, int width, int height)
{
    Graphics graphicController = null;

    try
    {
        if (originalImage != null)
        {
            int sourceWidth = originalImage.Width;
            int sourceHeight = originalImage.Height;
            int destX = 0;
            int destY = 0;

            float percent = 0;
            float percentW = 0;
            float percentH = 0;

            percentW = width / (float)sourceWidth;
            percentH = height / (float)sourceHeight;
            if (percentH < percentW)
            {
                percent = percentH;
                destX = Convert.ToInt16((width - (sourceWidth * percent)) / 2);
            }
            else
            {
                percent = percentW;
                destY = Convert.ToInt16((height - (sourceHeight * percent)) / 2);
            }

            int destWidth = (int)(sourceWidth * percent);
            int destHeight = (int)(sourceHeight * percent);

            Bitmap tempPhoto = new Bitmap(width, height, PixelFormat.Format24bppRgb);

            tempPhoto.SetResolution(originalImage.HorizontalResolution, originalImage.VerticalResolution);
            graphicController = Graphics.FromImage(tempPhoto);
            graphicController.FillRectangle(Brushes.White, 0, 0, width, height);

            // The chances of hitting this ONE SPECIFIC COLOR are very slim
            graphicController.SmoothingMode = SmoothingMode.HighQuality;
            graphicController.PixelOffsetMode = PixelOffsetMode.HighQuality;
            graphicController.InterpolationMode = InterpolationMode.HighQualityBicubic;
            graphicController.CompositingQuality = CompositingQuality.HighQuality;

            graphicController.DrawImage(
                originalImage,
                new Rectangle(destX, destY, destWidth, destHeight),
                new Rectangle(1, 1, sourceWidth - 1, sourceHeight - 1),
                GraphicsUnit.Pixel);

            return tempPhoto;
        }
        else
        {
            return null;
        }
    }
    catch
    {
        throw;
    }
    finally
    {
        graphicController.Dispose();
    }
}
\$\endgroup\$
5
\$\begingroup\$
  • Reversing the condition of if (originalImage != null) and returning early will remove one level of indentation. The less indentation some code shows the easier to read it will become.
  • catch to just rethrow, althought you did it in the correct way, doesn't buy you anything. Just remove the try..catch..finally and enclose the usage of graphics with an usage block.
  • Althought the code is mostly named well, one could stumble over tempPhoto and can't figure out at first glance wether this object is important or just temporary.
  • You can cast e.g ((width - (sourceWidth * percent)) / 2) directly to int you don't need a call to Convert.ToInt16() here.
  • percentW and percentH are first defined and on the next lines you assign a value to them. You should do it just at the declaration to save some (superflous) lines of code.
  • Instead of graphicController.FillRectangle(Brushes.White, 0, 0, width, height); you should use graphicController.Clear(Color.White); which is easier to read.
  • Comments, when used, should state, in a clear and understandable way, why the code is written as it is. I don't get the comment // The chances of hitting this ONE SPECIFIC COLOR are very slim. Where could be a problem with the code to make this comment justified?
  • The code validates the Image originalImage method parameter, but allows "illegal" values for int width and int height.

Implementing most of the mentioned points (validation is for you) will lead to

public static Image ImageToFixedSize(Image originalImage, int width, int height)
{
    if (originalImage == null) { return null; }

    int sourceWidth = originalImage.Width;
    int sourceHeight = originalImage.Height;
    int destX = 0;
    int destY = 0;

    float percent = 0;
    float percentW = width / (float)sourceWidth;
    float percentH = height / (float)sourceHeight;

    if (percentH < percentW)
    {
        percent = percentH;
        destX = (int)((width - (sourceWidth * percent)) / 2);
    }
    else
    {
        percent = percentW;
        destY = (int)((height - (sourceHeight * percent)) / 2);
    }

    int destWidth = (int)(sourceWidth * percent);
    int destHeight = (int)(sourceHeight * percent);

    Bitmap fixedSizedImage = new Bitmap(width, height, PixelFormat.Format24bppRgb);
    fixedSizedImage.SetResolution(originalImage.HorizontalResolution, originalImage.VerticalResolution);

    using (Graphics graphicController = Graphics.FromImage(fixedSizedImage))
    {
        graphicController.Clear(Color.White);

        // The chances of hitting this ONE SPECIFIC COLOR are very slim
        graphicController.SmoothingMode = SmoothingMode.HighQuality;
        graphicController.PixelOffsetMode = PixelOffsetMode.HighQuality;
        graphicController.InterpolationMode = InterpolationMode.HighQualityBicubic;
        graphicController.CompositingQuality = CompositingQuality.HighQuality;

        graphicController.DrawImage(
            originalImage,
            new Rectangle(destX, destY, destWidth, destHeight),
            new Rectangle(1, 1, sourceWidth - 1, sourceHeight - 1),
            GraphicsUnit.Pixel);
    }

    return fixedSizedImage;

}
\$\endgroup\$
  • 1
    \$\begingroup\$ I agree with almost everything you stated, thank you for your assistance. I want to ask, about the casting of sourceWidth / sourceHeight... My environment is warning me about a 'Possible loss of fraction'. Thoughts? \$\endgroup\$ – CarComp Oct 22 at 12:32
  • 1
    \$\begingroup\$ Well, my VS 2017 doesn't show a warning but you are right. One of the terms should be float. Will edit my answer. \$\endgroup\$ – Heslacher Oct 22 at 12:43
  • \$\begingroup\$ Just a note on the using: With the current C# version, you can just write using var graphicsController = Graphics.FromImage(...); without having a block after. It will automatically be disposed once the enclosing scope is finished. See here \$\endgroup\$ – germi Oct 22 at 19:33

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.