# Resizing image but keeping aspect ratio

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);
}

• What changes did you actually make? I cannot see any difference in the original and the end result Commented Nov 26, 2014 at 12:36
• Is this int rectWidth = BOXWIDTH; some copy paste error or should both rectWidth = rectHeight = BOXWIDTH be assigned to the same value ? Commented Nov 26, 2014 at 12:39
• it's a copy paste error yes, sorry, I will edit it Commented Nov 26, 2014 at 12:40
• 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. Commented Nov 26, 2014 at 14:59
• 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 Commented Nov 26, 2014 at 22:06

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));


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
{

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);
}
}

{
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 :)

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);


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;

• 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) Commented Nov 26, 2014 at 12:48
• 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. Commented Nov 26, 2014 at 12:50
• 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. Commented Nov 26, 2014 at 12:51
• You can't var the declarations for newWidth and newHeight unless you are assigning to them right?
– Evan
Commented Nov 27, 2014 at 0:49
• @Evan You're right, that was a mistake of me. Commented Nov 27, 2014 at 0:58