current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

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
}
share|improve this question

migrated from stackoverflow.com Sep 16 '11 at 13:53

This question came from our site for professional and enthusiast programmers.

4 Answers 4

up vote 8 down vote accepted

The code is not terrible, but I can make a few observations that we can use to improve the code:

  • It will be useful to know elsewhere (presentation time, perhaps?) whether or not this is portait or landscape. Let's build that now so we can keep the information somewhere useful.
  • The only ways any of those values will ever by 0 (not set) is if height or width is zero, or the image is too small. That's probably an invalid state to begin with, and it should the responsibility of the code that calls this to deal with it. So let's check for that up front.
  • I'm concerned about your use of "magic numbers": 471, 353, and 120. I'd love to see those factored out to variables.

With that in mind, here's an idea:

//there are better places to define this, but I'll leave them here now for convenience in this example
int minLandscapeWidth = 471, minPortiatHeight = 353, thumbLongSide = 120;

int width = img.Width;
int height = img.Height;

//you may not need these checks, depending what your img object is and how you use it
if (width <= 0) throw new InvalidOperationException("img.Width should be greater than zero");
if (height<= 0) throw new InvalidOperationException("img.Height should be greater than zero");

bool landscape = (width > height);

if ( (landscape && width < minLandscapeWidth) || (!landscape && height < minPortraitHeight) )
    throw new InvalidOperationException("the image is too small");

int thumbWidth, thumbHeight, preWidth, preHeight;

if (landscape)
{
    thumbWidth = thumbLongSide;
    thumbHeight = ((thumbLongSide * height) / width);
    preWidth = minLandscapeWidth;
    preHeight = ((minLandscapeWidth * height) / width);
}
else 
{
    thumbHeight = thumbLongSide;
    thumbWidth = ((thumbLongSide * width) / height);
    preHeight = minPortraitHeight ;
    preWidth = ((minPortraitHeight * width) / height);
}

//values are now set
share|improve this answer
1  
It's interesting that you come away from the original code thinking there are problems with images being too small, and I come away thinking that it intends to deal with images that are too big. I wonder if seeing the actual class/method name would help provide the correct context. @Chuchelo, this is an example of where I think you see the importance of descriptive names, because absent that, people are left to interpret what code might actually mean. –  Anthony Pegram Sep 16 '11 at 14:38
up vote 8 down vote

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.

share|improve this answer
    
+1 Nice: Like the idea of moving test to descriptive methods so that comments can be removed. –  Loki Astari Sep 16 '11 at 14:44
up vote 1 down vote

I think the logic flow reads better if you factor out the calculation of the thumb* and pre* variables:

private void Doit() {
  int width = img.Width;
  int height = img.Height;

  var dimensions=CalculateDimensions(width, height);

  //if values were set
  if(dimensions!=null) {

  } else {
    //do other stuff
  }
}

private static Dimension CalculateDimensions(int width, int height) {
  //if Landscape
  if(width > height && width >= 471) {
    return new Dimension(120, ((120*height)/width), 471, ((471*height)/width));
  }
  if(height > width && height >= 353) {
    return new Dimension(120, ((120*width)/height), 353, ((353*width)/height));
  }
  return null;
}

public sealed class Dimension {
  ...
}
share|improve this answer
up vote 0 down vote

How about this as a starter?

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

            bool valuesSet = false;

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

            //If values were set
            if (valuesSet)
            {
            }
            else 
            { 
                //do other stuff
            }
share|improve this answer
    
Thanks for the answer. I wouldn't really like to use flags for this purpose. Do you have any other suggestions ? Perhaps a new method that returns both values somehow. I just don't have enough experience yet. –  Chuchelo Sep 16 '11 at 14:03
    
That's a fair comment - your question was originally posted on StackOverflow; had I known we were migrating to CodeReview I would have suggested something more radical. You've got some great other suggestions on the page now. –  Sam Sep 16 '11 at 14:38
    
I appreciate the answer anyway, Sam ! –  Chuchelo Sep 16 '11 at 15:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.