Функция проверки проверки кода


Любые предложения по очистке этот код:

 public override bool Validate(Control control, object value)
    {
      bool res = false;
      string str = string.Empty;

      try
      {
        if (((string)value).Length > 0)
          str = value.ToString();
      }
      catch (Exception e)
      {
        res = false;
        this.ErrorText = "Bad values " + e.Message;
        this.ErrorType = ErrorType.Critical;
      }

      //Is it empty?
      if (String.IsNullOrEmpty(str))
      {
        res = false;
        this.ErrorType = ErrorType.Critical;
      }

      Regex RegNrShortRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3})");
      Regex RegNrLongRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3}[-][0-9]+$)");
      Regex RegNrShortRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3})");
      Regex RegNrLongRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3}[-][0-9]+$)");

      if (selectedItem == Constants.Item1)
      {
        this.ErrorText = "The correct form is: FAD-Zxxx oder FAD-Zxxx-x !";

        //has the form FAD-Zxxx
        if (RegNrShortRegexDarm.IsMatch(str) && str.Length == 8)
        {
          // Number is valid
          res = true;
        }

        //has the form FAD-Zxxx-x
        if (RegNrLongRegexDarm.IsMatch(str) && str.Length >= 10 && str.Length <= 12)
        {
          // Number is valid
          res = true;
        }
      }
      else if (selectedItem == Constants.Item2)
      {
        this.ErrorText = "The correct form is: FAB-Zxxx oder FAB-Zxxx-x !";

        //has the form FAB-Zxxx
        if (RegNrShortRegexBrust.IsMatch(str) && str.Length == 8)
        {
          // Number is valid
          res = true;
        }

        //has the form FAB-Zxxx-x
        if (RegNrLongRegexBrust.IsMatch(str) && str.Length >= 10 && str.Length <= 12)
        {
          // Number is valid
          res = true;
        }
      }

      return res;
    }

Позже Редактировать:

Я пришел к следующему решению:

public override bool Validate(Control control, object value)
{
  var str = value as string;
  var letter = (selectedItem == Constants.Item1) ? "D" : "B";
  var errText = String.Format("The correct form is: FA{0}-Zxxx or FA{0}-Zxxx-x !", letter);

  if (string.IsNullOrWhiteSpace(str))
  {
    this.ErrorType = ErrorType.Critical;
    this.ErrorText = errText;
    return false;
  }

  string regExString = String.Format(@"FA{0}-Z\d{{3}}", letter);
  Regex shortRegEx = new Regex(regExString);
  Regex longRegEx = new Regex(regExString + @"-\d+$");

  bool isMatch = ((shortRegEx.IsMatch(str) && IsValidShortLength(str)) || (longRegEx.IsMatch(str) && IsValidLongLength(str)));

  if (!isMatch)
  {
    this.ErrorText = errText;
  }
  return isMatch;

}

private static bool IsValidShortLength(string str)
{
  return str.Length == 8;
}

private static bool IsValidLongLength(string str)
{
  return (str.Length >= 10 && str.Length <= 12);
}


1617
2
c#
задан 15 сентября 2011 в 12:09 Источник Поделиться
Комментарии
5 ответов

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

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

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

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

        var str = value as String;

var letter = (selectedItem == Constants.Item1) ? "D" : "B";
var errText = String.Format("The correct form is: FA{0}-Zxxx oder FA{0}-Zxxx-x !", letter);

if (str == null
|| str.Length < 8
|| !(str.Length >= 10 && str.Length <=12))
{
this.ErrorType = ErrorType.Critical;
this.ErrorText = errText;
return false;
}

var regExString = @"([F][A][" + letter + @"][-][Z]\d{3})";
Regex shortRegEx = new Regex(regExString);
Regex longRegEx = new Regex(regExString + @"[-][0-9]+$)");

var isMatch = (shortRegEx.IsMatch(str) || longRegEx.IsMatch(str));
if (!isMatch)
{
this.ErrorText = errText;
}

return isMatch;

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

Просто нужно подобрать по регулярные выражения (так как другие люди собирают некоторые другие вещи я бы прокомментировал):

  Regex RegNrShortRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3})");
Regex RegNrLongRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3}[-][0-9]+$)");
Regex RegNrShortRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3})");
Regex RegNrLongRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3}[-][0-9]+$)");

Просто угробить классов для одного персонажа: [Ф] - это перебор. И быть последовательным в классе для цифр. Кроме того, нет смысла использовать группу для всего выражения.

  Regex RegNrShortRegexDarm = new Regex(@"FAD-Z\d{3}");
Regex RegNrLongRegexDarm = new Regex(@"FAD-Z\d{3}-\d+$");
Regex RegNrShortRegexBrust = new Regex(@"FAB-Z\d{3}");
Regex RegNrLongRegexBrust = new Regex(@"FAB-Z\d{3}-\d+$");

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

  Regex RegNrDarm = new Regex(@"FAD-Z\d{3}(-\d{1,3})?");
Regex RegNrBrust = new Regex(@"FAB-Z\d{3}(-\d{1,3})?");

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

Я не могу поверить, что говорю это, но у вас слишком много обработки ошибок происходит. Первые 15 строк или так все сделки с преобразования значения в строку.

просто сделать что-то вроде...

string str = value as string;
if(string.IsNullOrWhiteSpace(str))
{
this.ErrorType = ErrorType.Critical;
return false;
}

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

Название переменных в соответствии с инструкцией№. Все переменные должны быть в стиле Camel, не в стиле Pascal.

Используйте строку псевдоним, а не строку.

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

После объединения некоторые из других ответов и незначительные стилистические хитрости, мы получаем:

 public override bool Validate(Control control, object value)
{
var str = value as string;
var letter = (selectedItem == Constants.Item1) ? "D" : "B";
var errText = String.Format("The correct form is: FA{0}-Zxxx oder FA{0}-Zxxx-x !", letter);

if (string.IsNullOrEmpty(str))
{
this.ErrorType = ErrorType.Critical;
this.ErrorText = errText;
return false;
}

string pattern = @"(FA" + letter + @"-Z\d{3}(-\d{1,3})?";
bool isMatch = Regex.IsMatch(str, pattern);

if (!isMatch)
{
this.ErrorText = errText;
}
return isMatch;
}

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

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

Я бы избежать смешения языков в моем сообщении об ошибке. Английское слово "или", а не "Одер", который является немецким. Было бы лучше, чтобы интернационализировать сообщения об ошибках, которые подразумевают получение, что формат строки из файла ресурсов, а не хардкодить его.

Вышесказанное, наверное, иначе становится довольно близкой к оптимальной. Эта функция тоже не совсем такой, как оригинал, поскольку он делает предположение, что элемент - это всегда либо константы.Элемент1 или константы.Место № 2. Если это изменение не допустимо, выше метод должен быть соответствующим образом изменен.

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

Другие люди комментировали ваш попробовать/поймать и стоит ли это даже применимо, но я не верю, как уже упоминалось, вы не должны поймать исключение, а скорее то, что вы можете реально справиться, и следовало ожидать. Учитывая ваш код, я бы ожидать, чтобы нарваться на исключение NullReferenceException или исключение invalidcastexception. Дело в том, вы можете избежать и тех и тех, Но если вы не могли, бы те типы вы хотели бы поймать. Другие исключения должны пузыриться.

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

Другое дело, что я вижу, это

if (selectedItem == x)
{
// several lines of code
}
else if (selectedItem == y)
{
// several lines of code
}

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

Я вижу, что в верхней части кода, который вы установите рез = ложь , когда НТР является нулем или пустой. Но позже вы можете установить РЭС в true. Ли это даже возможно для последующих разделов кода для переопределения начального значения, я не хочу даже думать об этом. Если это ОК, чтобы переопределить, это одно. Если это не так (даже если это даже не возможно), не заставляй меня выполнять умственную гимнастику, чтобы подтвердить себе, что значение не изменится.

Мой (надеюсь) последнее замечание я вижу это в нескольких местах

// Number is valid
res = true;

Вы знаете, что этот комментарий говорит мне? Он говорит мне, что РЭС - это ужасное имя для переменной. РЭС говорит мне ничего. Переименовать переменную, чтобы что-то, что вообще-то мне подсказывает, какую информацию она должна передать, и тогда вы можете избавиться от бесполезных комментариев.

isValidNumber = true; 

ОК, так что напрямую подводит меня к другой точке.

//Is it empty?       
if (String.IsNullOrEmpty(str))

Я думаю?

Мне не нужен комментарий, чтобы сказать мне, что строка кода говорит мне. Сохраните свои комментарии на вещи, которые действительно должны быть объяснены. Возможно, комментарии будут полезными для регулярных выражений, потому что, кто хочет думать о них. Предоставить комментарии высокого уровня для сложных кусков кода, или скажите, почему вы выбрали алгоритм X над Y, если вы чувствуете необходимость. Не давать замечания за то, что код уже говорит мне (и рефакторинг кода, когда код должен быть в состоянии сказать мне, что мне нужно знать, как и в случае с РЭС).

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