13
\$\begingroup\$

I wrote some code that resizes an image to fit in a given box. I think that some parts of the code are not needed (for example if it's a box image). Is there a more efficient way to do this? Is there something wrong with my code (from the results it looks ok)? Is there a "shorter" version to do this?

Bitmap original, resizedImage;

try
{
    using (FileStream fs = new System.IO.FileStream(imageLabel.Text, System.IO.FileMode.Open))
    {
        original = new Bitmap(fs);
    }

    int rectHeight = BOXHEIGHT;
    int rectWidth = BOXWIDTH;

    //if the image is squared set it's height and width to the smallest of the desired dimensions (our box). In the current example rectHeight<rectWidth
    if (original.Height == original.Width)
    {
        resizedImage = new Bitmap(original, rectHeight, rectHeight);
    }
    else
    {
        //calculate aspect ratio
        float aspect = original.Width / (float)original.Height;
        int newWidth, newHeight;

        //calculate new dimensions based on aspect ratio
        newWidth = (int)(rectWidth * aspect);
        newHeight = (int)(newWidth / aspect);

        //if one of the two dimensions exceed the box dimensions
        if (newWidth > rectWidth || newHeight > rectHeight)
        {
            //depending on which of the two exceeds the box dimensions set it as the box dimension and calculate the other one based on the aspect ratio
            if (newWidth > newHeight)
            {
                newWidth = rectWidth;
                newHeight = (int)(newWidth / aspect);
            }
            else
            {
                newHeight = rectHeight;
                newWidth = (int)(newHeight * aspect);
            }
        }
        resizedImage = new Bitmap(original, newWidth, newHeight);
    }
}
catch (Exception ex)
{
    MessageBox.Show(ex.Message);
}
\$\endgroup\$
  • \$\begingroup\$ What changes did you actually make? I cannot see any difference in the original and the end result \$\endgroup\$ – John Demetriou Nov 26 '14 at 12:36
  • \$\begingroup\$ Is this int rectWidth = BOXWIDTH; some copy paste error or should both rectWidth = rectHeight = BOXWIDTH be assigned to the same value ? \$\endgroup\$ – Heslacher Nov 26 '14 at 12:39
  • \$\begingroup\$ it's a copy paste error yes, sorry, I will edit it \$\endgroup\$ – John Demetriou Nov 26 '14 at 12:40
  • \$\begingroup\$ Do not forget that the Bitmap class implements IDisposable (like FileStream does) and your original and resizedImage members will need to be properly Dispose()'d somewhere. \$\endgroup\$ – Jesse C. Slicer Nov 26 '14 at 14:59
  • \$\begingroup\$ I went through a refactoring exercise of some resizing code of mine a while back in accordance with the style described in the Clean Code book. You can take a look and see if it can provide some inspiration: without-precedence.dk/articles/32/Writing-Clean-Code \$\endgroup\$ – Morten Christiansen Nov 26 '14 at 22:06
12
\$\begingroup\$

Your algorithm to calculate the final image dimensions could be improved by simplifying the logical process that you use to determine the final size.

Fundamentally, there are two dimensions you are interested in, the height, and width of the final image. There are two possible outcomes for the scaling, one where the scaling produces the target height, and the other where it produces the target width. The scaling that is used is the one which produces the smallest image. The size of the final image is determined by the scaling factor, and there are two factors:

float scaleHeight = (float)BOXHEIGHT / (float)original.Height;
float scaleWidth = (float)BOXWIDTH / (float)original.Width;

Now, we have two scaling ratios, which one produces the smaller image? The one that has the smallest scaling factor.

float scale = Math.Min(scaleHeight, scaleWidth);

Now you can simply do the adjustment:

resizedImage = new Bitmap(original,
         (int)(original.Width * scale), (int)(original.Height * scale));
\$\endgroup\$
4
\$\begingroup\$

Here's how you can refactor your code a bit:

namespace ConsoleApplication1
{
    using System;
    using System.Drawing;
    using System.IO;

    public class Program
    {
        public static void Main(string[] args)
        {
        }

        private const int BOXWIDTH = 1;

        public static Bitmap Foo()
        {    
            try
            {
                Bitmap original = LoadOriginalImage(imageLabel.Text);

                if (ImageIsBox(original))
                    return new Bitmap(original, BOXWIDTH, BOXWIDTH);

                var newDimensions = CalculateNewDimensionsForImage(original);

                return new Bitmap(original, newDimensions.Width, newDimensions.Height);
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
        }

        private static Bitmap LoadOriginalImage(string imagePath)
        {
            using (FileStream fs = new FileStream(imagePath, FileMode.Open))
            {
                return new Bitmap(fs);
            }
        }

        private static bool ImageIsBox(Bitmap original)
        {
            return original.Height == original.Width;
        }

        private static ImageDimensions CalculateNewDimensionsForImage(Bitmap original)
        {
            //calculate aspect ratio
            float aspect = original.Width / (float)original.Height;
            int newWidth, newHeight;

            //calculate new dimensions based on aspect ratio
            newWidth = (int)(BOXWIDTH * aspect);
            newHeight = (int)(newWidth / aspect);

            //if one of the two dimensions exceed the box dimensions
            if (newWidth > BOXWIDTH || newHeight > BOXWIDTH)
            {
                //depending on which of the two exceeds the box dimensions set it as the box dimension and calculate the other one based on the aspect ratio
                if (newWidth > newHeight)
                {
                    newWidth = BOXWIDTH;
                    newHeight = (int)(newWidth / aspect);
                }
                else
                {
                    newHeight = BOXWIDTH;
                    newWidth = (int)(newHeight * aspect);
                }
            }

            return new ImageDimensions()
            {
                Height = newHeight,
                Width = newWidth
            };
        }
    }

    public class ImageDimensions
    {
        public int Height { get; set; }
        public int Width { get; set; }
    }
}

I've written a console app that tries to illustrate how you could simplify your code a bit. The end result is that a lot of variables that were using are now gone e.g. resizedImage, rectHeight, rectWidth.

This might be useful for you, if not, no worries, I enjoyed writing it :)

\$\endgroup\$
3
\$\begingroup\$

Style

  • Declaring multiple variables on the same line should be avoided for the benefit of readability

  • Using braces {} for single if statements also is good

  • Using meaningful names for variables is good

General

This code is tightly coupled to Windows Forms or WPF (imageLabel) which isn't needed. Also it is coupled to BOXWIDTH constant. The method should be static as it doesn't change any member variables.

Your method signature should look like

private static Bitmap GetResizedImage(String fileName, Int32 maxWidth, Int32 maxHeight)
{

}   

and be called like

Bitmap resized = GetResizedImage(imageLabel.Text, BOXWIDTH, BOXHEIGHT);
\$\endgroup\$
3
\$\begingroup\$

Your code looks pretty neat to me. I have a few remarks listed below.

Variable names:

Don't mix up names, this makes things confusing:

int rectHeight = BOXWIDTH;
int rectWidth = BOXWIDTH;

Change it to:

int boxHeight = BOXWIDTH;
int boxWidth = BOXWIDTH;

Now you know you're talking about the box and not mistaking it for some possible rectangle.

Don't uppercase field names, use pascalCase. Also, aspect is not a very good name, choose a name like ratio or aspectRatio.

var keyword:

Use var instead of declaring your varibales explicitly, let the compiler decide the type. Your code will also look cleaner:

var aspect = original.Width / (float)original.Height;
\$\endgroup\$
  • \$\begingroup\$ I use uppercase for constants, yeah I should have mentioned that aswell. BUt what about code efficiency or if parts of the code are not needed (code logic) \$\endgroup\$ – John Demetriou Nov 26 '14 at 12:48
  • 2
    \$\begingroup\$ I disagree about the var advise. It's subjective, but declaring everything var actually makes things more confusing for me. If something is a int declare it as so. But that's just me. \$\endgroup\$ – Jason Evans Nov 26 '14 at 12:50
  • \$\begingroup\$ I don't see any flaws regarding efficiency and as far as I can tell none of the logic is superfluous. I'll try to test your code later today. \$\endgroup\$ – Abbas Nov 26 '14 at 12:51
  • \$\begingroup\$ You can't var the declarations for newWidth and newHeight unless you are assigning to them right? \$\endgroup\$ – Evan Nov 27 '14 at 0:49
  • \$\begingroup\$ @Evan You're right, that was a mistake of me. \$\endgroup\$ – Abbas Nov 27 '14 at 0:58

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.