durgeshtupkar10 Ответов: 3

Как сократить следующий код

public bool ValidCompulsoryFields()
    {
        if (ItemTypeName.Text == string.Empty || ItemTypeName.Text == StringConstants.select ||
                    ItemTypeName.SelectedValue.ToString() == StringConstants.select)
        {
            this.Master.LblError.Text= ErrorConstants.selectItemType;
            this.Master.LblError.ForeColor = Color.Red;
            ItemTypeName.Focus();
            return false;
        }

        if (ItemCategoryName.Text == string.Empty || ItemCategoryName.Text == StringConstants.select ||
                    ItemCategoryName.SelectedValue.ToString() == StringConstants.select)
        {
            this.Master.LblError.Text=ErrorConstants.selectItemCat;
            this.Master.LblError.ForeColor = Color.Red;
            ItemCategoryName.Focus();
            return false;
        }

        if (ManufacturerName.Text == string.Empty || ManufacturerName.Text == StringConstants.select ||
               ManufacturerName.SelectedValue.ToString() == StringConstants.select)
        {
            this.Master.LblError.Text=ErrorConstants.selectMake;
            this.Master.LblError.ForeColor = Color.Red;
            ManufacturerName.Focus();
            return false;
        }

        if (PackagingByName.Text == string.Empty || PackagingByName.Text == StringConstants.select ||
               PackagingByName.SelectedValue.ToString() == StringConstants.select)
        {
           this.Master.LblError.Text=ErrorConstants.selectPackage;
           this.Master.LblError.ForeColor = Color.Red;
                
PackagingByName.Focus();
           return false;
        }
        else
            return true;

3 Ответов

Рейтинг:
20

OriginalGriff

Предполагая, что "ItemTypeName", "ItemCategoryName" и т. д. являются текстовыми полями или подобными им, рефакторируйте их в метод:

private bool CheckValidity(TextBox tb, ErrorConstant fail)
   {
   if (tb.Text == string.Empty || tb.Text == StringConstants.select ||
               tb.SelectedValue.ToString() == StringConstants.select)
       {
       this.Master.LblError.Text= fail;
       this.Master.LblError.ForeColor = Color.Red;
       tb.Focus();
       return false;
       }
   return true;
   }
Тогда просто позвоните ему несколько раз:
return CheckValidity(ItemTypeName,ErrorConstants.selectItemType) &&
       CheckValidity(ItemCategoryName,ErrorConstants.selectItemCat) &&
       CheckValidity(ManufacturerName,ErrorConstants.selectMake) &&
       CheckValidity(PackagingByName,ErrorConstants.selectPackage);

- Но здесь у меня четыре разных текстовых поля.
Тогда как я могу взять их в одном "если утверждение"?

на самом деле у меня есть 4 поля со списком а не текстовые поля"

TextBox/ComboBox - это не имеет никакого значения! Просто измените тип параметра.

Когда вы делаете такое заявление, как
bool b = bool1 && bool2 && bool3;
он оценивает их в строгом порядке: сначала Bool1. Если это так, оцените bool2. Если это не так, не идите дальше, потому что все выражение ложно.
Как только компилятор доберется до точки, где один false встречается, он прекращает обработку.
Так:
return CheckValidity(ItemTypeName,ErrorConstants.selectItemType) &&
       CheckValidity(ItemCategoryName,ErrorConstants.selectItemCat) &&
       CheckValidity(ManufacturerName,ErrorConstants.selectMake) &&
       CheckValidity(PackagingByName,ErrorConstants.selectPackage);
будет возвращать true если и только если все ваши текстовые поля / комбо-боксы проходят проверку. Если какой-либо из них выйдет из строя, он прекратит обработку и вернется false;

Sergey Alexandrovich Kryukov

Выглядит неплохо, мой 5-й.
Я решил держаться подальше от крысиных бегов, дал общий совет, как вы думаете?
--СА

Manfred Rudolf Bihy

Именно это я и имел в виду. За исключением того, что мой первый пост был каким-то образом искорежен. 5+
Ты ведь не увлекаешься этим чтением мыслей, правда? :)

OriginalGriff

Я пытался предсказывать судьбу, но не видел в этом никакого будущего...

durgeshtupkar10

Но здесь у меня есть четыре разных текстовых поля.
Тогда как я могу взять их в одном "если утверждение"?

durgeshtupkar10

на самом деле у меня есть 4 поля со списком а не текстовые поля

OriginalGriff

Ответ обновлен

Espen Harlinn

Хорошее усилие, мой 5-й

Рейтинг:
20

Sergey Alexandrovich Kryukov

Слишком скучно устраивать крысиные бега между экспертами, которые могут сделать самый короткий код, Но да, ваш код неоправданно длинный.

Помните, что ваша цель состоит не в том, чтобы сделать код закороченным, а в том, чтобы улучшить ремонтопригодность и читабельность. Эта цель изменила бы код в том же направлении (сделав его короче), но только до определенной степени. Главный принцип здесь-D. R. Y. (не повторяйтесь — http://en.wikipedia.org/wiki/Do_not_repeat_yourself[^]).

Вы повторяете. Смотреть на Color.Red, то же самое во всех ветвях — носите его наш из if блоки. Условия почти одинаковые — найдите общий знаменатель. Создайте функцию-предикат, проверяющую условие; другая часть условия будет параметром: это Text эфира PackagingByName, ManufacturerName, ItemCategoryName или ItemTypeName При использовании предиката каждое условие будет представлять собой одну короткую строку (вызов предиката с параметром). Что касается предикатов, то вы можете попробовать использовать мой метод trick — inner в анонимных формах; см. мои советы/статью Trick: Скрыть специальные методы внутри тела вызывающего метода[^] (некоторые читатели находят мой метод не читаемым, но я думаю, что это так — вы решаете).

Еще один общий совет: попробуйте сформулировать шнурки логических проверок неформально и реализовать ближе к этой формулировке.

—СА

Manfred Rudolf Bihy

Мудрый совет, СА! 5+

Sergey Alexandrovich Kryukov

Спасибо, Манфред.
--СА

OriginalGriff

Хороший совет!

Sergey Alexandrovich Kryukov

- Спасибо, Грифф.
--СА

Michel [mjbohn]

хороший Совет. Мой 5-й
Я также предпочитаю давать (и получать) общие советы. На мой взгляд, общий совет гораздо лучше подходит для обучения, чем готовое решение. Это может быть неправильное впечатление, но я думаю, что общий совет часто игнорируется или даже отвергается спрашивающим.

Sergey Alexandrovich Kryukov

Спасибо. Ваши заметки очень интересны.
--СА

Espen Harlinn

Хороший ответ, 5ed!

Sergey Alexandrovich Kryukov

Спасибо, Эспен.
--СА

Рейтинг:
10

Manfred Rudolf Bihy

Один из способов наверняка состоял бы в том, чтобы разложить функцию, которая делает все это по линиям 

public bool TestSetError(ComboBox control, String error)
{
    bool retVal = true;
    if (control.Text == string.Empty || control.Text == StringConstants.select ||
        control.SelectedValue.ToString() == StringConstants.select)
    {
        this.Master.LblError.Text= error;
        this.Master.LblError.ForeColor = Color.Red;
        control.Focus();
        retVal = false;
    }
 
}


return (TestSetError(ItemTypeName,     ErrorConstants.selectItemType) &&
        TestSetError(ItemCategoryName, ErrorConstants.selectItemCat) &&
        TestSetError(ManufacturerName, ErrorConstants.selectMake) &&
        TestSetError(PackagingByName,  ErrorConstants.selectPackage)
       );

Sergey Alexandrovich Kryukov

???

Manfred Rudolf Bihy

Каким-то образом мой пост был искорежен. Я возвращаюсь туда прямо сейчас! :)

Manfred Rudolf Bihy

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

Sergey Alexandrovich Kryukov

Выглядит неплохо, моя пятерка.
--СА

Espen Harlinn

Мой 5-й

Manfred Rudolf Bihy

Не удалось поймать вариант ErrorConstant, исправлено!