HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Checking for minimum image dimensions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
imagecheckingminimumdimensionsfor

Problem

This code is part of one of the methods. I'm pretty sure it is really bad programming. The third if statement is supposed to be called if all 4 variables were set. Is another method needed? How would you write this piece?

int width = img.Width;
int height = img.Height;
int thumbWidth = 0 , thumbHeight = 0;
int preWidth = 0, preHeight = 0;

//if Landscape
if (width > height && width >= 471)
{
    thumbWidth = 120;
    thumbHeight = ((120 * height) / width);
    preWidth = 471;
    preHeight = ((471 * height) / width);
}
//if portrait 
else if (height > width && height >= 353)
{
    thumbHeight = 120;
    thumbWidth = ((120 * width) / height);
    preHeight = 353;
    preWidth = ((353 * width) / height);
}

//If values were set
if (thumbWidth != 0 && thumbHeight != 0 && preWidth != 0 && preHeight != 0)
{

}
else 
{ 
    //do other stuff
}

Solution

The problems I see here involve code duplication and magic values, and perhaps some incomplete logic.

You use the numbers 120, 471, 353. I don't know what those mean. And they're duplicated. Make those variables with descriptive names.

Now let's look at this:

//if Landscape
if (width > height && width >= 471)


This is more than than just if landscape. This is if landscape and is too big. But I'm not encouraging you to write a better comment! Instead, I encourage you to encapsulate that conditional into it's own method, with a more descriptive name, and then remove the comment.

private bool IsLandscapeAndTooBig(width, height)
{
    return width > height && width > maximumWidth; // notice the new variable?
}


The same idea applies to your "if portrait" piece.

Let's look inside those ifs now. Again, the problem is the magic values, but perhaps these inner blocks need to be methods, perhaps ResizeLandscape and ResizePortrait (but you probably want to think a bit longer on these names). And instead of operating on 4 local variables, perhaps they should instead return a single encapsuting data object.

class ImageDimension
{
     public int ThumbWidth { get; set; }
     // continues 
}


one other thing I see is that you build thumbnail dimensions for too large of a landscape and too large of a portrait, but you don't seem to build them for images that are not too large. Does something just smaller than your threshold for "large" not need a thumbnail? This is where I think you might have some missing logic. On the other hand, you might not, I don't know entirely what you are trying to do. Just keep this in mind.

Code Snippets

//if Landscape
if (width > height && width >= 471)
private bool IsLandscapeAndTooBig(width, height)
{
    return width > height && width > maximumWidth; // notice the new variable?
}
class ImageDimension
{
     public int ThumbWidth { get; set; }
     // continues 
}

Context

StackExchange Code Review Q#4865, answer score: 8

Revisions (0)

No revisions yet.