Проверка на минимальные размеры изображения


Этот код является частью одного из методов. Я уверен, что это действительно плохо программирования. Третий оператор if должен быть вызван, если все 4 переменные были установлены. Нужен еще один способ? Как бы вы написать это произведение?

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
}


782
6
задан 16 сентября 2011 в 01:09 Источник Поделиться
Комментарии
4 ответа

Код не страшный, но я могу сделать несколько замечаний, которые мы можем использовать, чтобы улучшить код:


  • Это будет полезно знать в другом месте (время презентации, возможно?) является ли это портрет или пейзаж. Давайте строить, что сейчас, чтобы мы могли сохранить информацию где-то полезно.

  • В только стороны любого из этих значений будет либо 0 (не установлен) - если высота или ширина равна нулю, или изображение слишком маленькое. Это, наверное, недопустимом состоянии, чтобы начать с, и он должен ответственность за код, который вызывает это, чтобы справиться с ней. Так давайте проверим, что впереди.

  • Меня беспокоит использование "магических чисел": 471, 353, и 120. Я бы хотела посмотреть на тех разделены на переменные.

Имея это в виду, вот идея:

//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

8
ответ дан 16 сентября 2011 в 02:09 Источник Поделиться

Проблемы я вижу здесь предполагают дублирования кода и магических значений, и, возможно, какая-то неполная логика.

Вы используете цифры 120, 471, 353. Я не знаю, что это значит. И они дублируются. Сделать эти переменные с именами.

Теперь давайте посмотрим на это:

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

Это более чем просто , если пейзаж. Это если пейзаж и слишком большой. Но я не призываю вас написать лучший комментарий! Вместо этого, я призываю вас, чтобы инкапсулировать, что условные в свой собственный метод, более описательное имя, а потом удалить комментарий.

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

Та же идея относится и к вашему "если портрет" кусок.

Давайте заглянем внутрь этих ИФС сейчас. Опять же, проблема волшебного значения, но, возможно, эти внутренние блоки должны быть методы, возможно, ResizeLandscape и ResizePortrait (но вы, вероятно, хотите, чтобы думать немного больше на эти имена). И вместо того, чтобы оперировать на 4 локальных переменных, возможно, они должны возвращать один encapsuting объект данных.

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

одна вещь, которую я вижу, заключается в том, что вы строите эскиз размеры слишком большие пейзажа и слишком большие портрета, но вы, кажется, не строить их для изображений, которые не слишком большие. Делает что-то просто меньше, чем порог для "большой" не нужен эскиз? Здесь, я думаю, вы могли бы иметь некоторые отсутствует логика. С другой стороны, вы не могли бы, я не совсем понимаю, что вы пытаетесь сделать. Просто имейте это в виду.

8
ответ дан 16 сентября 2011 в 02:09 Источник Поделиться

Я думаю, что логика гласит Лучше, если вы фактор в расчет большого пальца* и до* переменные:

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 {
...
}

1
ответ дан 16 сентября 2011 в 02:09 Источник Поделиться

Как насчет этого в качестве закваски?

            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
}

0
ответ дан 16 сентября 2011 в 01:09 Источник Поделиться