Govnokodu.Net

Дублирование реализации и неправильная инкапсуляция при проверке терма

Исходный код

private int checkTerm(PatternTerm term, int arr[],
      int symbNum, boolean isEditing) {
    if (term.count != INFINITY) {
      intendIdx= Math.min(symbNum + term.count, arr.length);
        intcount=0;
        switch(term.termType) {
          case DIGIT: {
            for (inti= symbNum; i < endIdx; i++) {
              if (!Character.isDigit((char)arr[i]))
                return -1;
              count++;
            }
            break;
          }
          case LETTER: {
            for(inti= symbNum; i < endIdx; i++) {
              if (!Character.isLetter((char)arr[i]))
                return -1;
              count++;
            }
            break;
          }
          case LETTERORDIGIT: {
            for(inti= symbNum; i < endIdx; i++) {
              if (!Character.isLetterOrDigit((char)arr[i]))
                return -1;
              count++;
            }
            break;
          }
          case CHARACTER: {
            for(inti= symbNum; i < endIdx; i++) {
              if (arr[i] != term.value)
                return -1;
              count++;
            }
            break;
          }
      }
      if (!isEditing && count != term.count)
        return -1;
      
      symbNum+=term.count;
    } else {
      inti=0;
      switch(term.termType) {
        case DIGIT: {
          while (((symbNum + i) < arr.length) &&
              Character.isDigit((char)arr[symbNum + i])) i++;
          break;
        }
        case LETTER: {
          while (((symbNum + i) < arr.length) &&
              Character.isLetter((char)arr[symbNum + i])) i++;
          break;
        }
        case LETTERORDIGIT: {
          while (((symbNum + i) < arr.length) &&
              Character.isLetterOrDigit((char)arr[symbNum + i])) i++;
          break;
        }
        case CHARACTER: {
          while (((symbNum + i) < arr.length) &&
              (arr[symbNum + i] == term.value)) i++;
          break;
        }
      }
      symbNum+=i;
    }
    return symbNum;
  }

Что не так в исходном коде

Первое, что бросилось в глаза: альтернативные действия в алгоритме выбираются в зависимости от типа объекта-параметра. Согласно паттерну «информационный эксперт» из GRASP, если детали алгоритма зависят от другого объекта, то есть смысл алгоритм «отдать» тому другому объекту.

В нашем случае проверка типа симоволов (Character.isDigit, Character.isLetter, Character.isLetterOrDigit, Character.isLetterOrDigit) зависит от типа терма (term.termType), так что правильнее будет эту проверку перенести в код класса терма.

Второй момент — повторяющаяся структура циклов:

            for (int i= symbNum; i < endIdx; i++) {
              if (!Character.isDigit((char)arr[i]))
                return -1;
              count++;
            }

и

            while (((symbNum + i) < arr.length) &&
              Character.isDigit((char)arr[symbNum + i])) i++;

В этой структуре заключена реализация обхода строки символов, реализация этого обхода не должна дублироваться.

Третья вещь — подозрительное сходство алгоритмов в ветках для term.count:

    if (term.count != INFINITY) {
      // ...
    }
    else {
      // ...
    }

И там, и там — обход массива и ветвление по типу терма.

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

Вариант рефакторинга исходного кода

class PatternTerm
{
  public int checkTerm(int[] text, int offset, boolean isEditing) {
    if (count == INFINITY) {
      return checkEndless(text, offset, isEditing);
    }
    else {
      return checkEnded(text, offset, isEditing);
    }
  }

  private int checkEnded(int[] text, int offset, boolean isEditing) {
    int endIdx= Math.min(offset + count, text.length);
    for (int i= offset; i < endIdx; i++) {
      boolean isMatched = checkChar(text[i]);
      if (!isMatched) {
        return -1;
      }
    }
    return endIdx;
  }

  private int checkEndless(int[] text, int offset, boolean isEditing) {
    int i= offset;
    while (i < text.length && isMatched(text[i])) i++;
    return i;
  }

  private boolean checkChar(char c) {
    switch (termType) {
      case DIGIT: {
        return Character.isDigit(c);
      }
      case LETTER: {
        return Character.isLetter(c);
      }
      case LETTERORDIGIT: {
        return Character.isLetterOrDigit(c);
      }
      case CHARACTER: {
        return c== value;
      }
    }
    return false;
  }
}

А ещё было бы здорово создать иерархию термов, где был бы родитель — обобщённый терм, и два наследника: терм с известной длиной и терм с неизвестной длиной, и у наследников одинаковый метод checkTerm, только у одного с телом checkEnded, а у другого — checkEndless. Но тут уже придётся сильнее вмешиваться в код, создающий термы.

Теория

Сергей Галанин