Resizing image but keeping aspect ratio

Question

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

• John Demetriou: What changes did you actually make? I cannot see any difference in the original and the end result
• Heslacher: Is this int rectWidth = BOXWIDTH; some copy paste error or should both rectWidth = rectHeight = BOXWIDTH be assigned to the same value ?
• John Demetriou: it's a copy paste error yes, sorry, I will edit it
• Jesse C. Slicer: 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.
• Morten Christiansen: 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

Answer 1

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

Answer 2

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

Answer 3

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

Answer 4

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;

• John Demetriou: 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)
• Jason Evans: 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.
• Abbas: 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.
• Evan: You can't var the declarations for newWidth and newHeight unless you are assigning to them right?
• Abbas: @Evan You're right, that was a mistake of me.