Govnokodu.Net

Пример рефакторинга «извлечение метода» для алгоритма поиска точек в массиве

Исходный код

foreach (clsPoint Point in ArrPoint) {
    if (!f1 && (P1.DistanceToPoint (p) > Point.DistanceToPoint (p) || P1.DistanceToPoint (p) < 1)
        && (P2 == null || p.isInToLine (Point, P2) && Point.DistanceToPoint (P2) > 1) &&
        (fc || Point1.Classify (P2.GetConnectBeem (this)[0].Point1, P2) ==
                        Point.Classify (P2.GetConnectBeem (this)[0].Point1, P2))) {
        P1 = Point;
        continue;
    }
    if (!f2 && (P2.DistanceToPoint (p) > Point.DistanceToPoint (p) || P2.DistanceToPoint (p) < 1)
        && (P1 == null || p.isInToLine (Point, P1) && Point.DistanceToPoint (P1) > 1) &&
        (fc || Point2.Classify (P1.GetConnectBeem (this)[0].Point2, P1) ==
                        Point.Classify (P1.GetConnectBeem (this)[0].Point2, P1))) {
        P2 = Point;
        continue;
    }
}

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

Не очень понятно, что происходит в исходном коде, хотя и как происходит — тоже не сильно ясно.

Навскидку здесь поиск двух специфических точек P1, P2 в массиве ArrPoint, причём этот поиск инкрементный и взаимозависимый (значения с предыдущей итерации влияют на следующую, P1 влияет на P2 и наоборот).

Чтобы начать распутывать этот клубок, разберусь сначала с данными. Итак, на выходе видим две переменных P1 и P2. А что на входе?

  • ArrPoint — исходный массив, в котором ищем подходящие точки
  • f1, f2, fc — некие условия, вычисленные раньше
  • p, Point1, Point2 — некие опорные точки

Первый существенный недостаток — ни одно название переменной ничего не сообщает о смысле и предназначении значения, которое в ней хранится. Соответственно, первый шаг — переименовать все переменные.

Второй осложняющий жизнь момент — не ясен смысл проверок P1.DistanceToPoint (p) > Point.DistanceToPoint (p) || P1.DistanceToPoint (p) < 1, P2 == null || p.isInToLine (Point, P2) && Point.DistanceToPoint (P2) > 1 и т. д.

Назвать их смысл можно только с помощью идентификаторов (комментарии не рассматриваю из принципа), то есть с помощью одного из рефакторингов «извлечение метода» или «замена временной переменной на вызов метода». Так как структура проверок для обоих операторов if сходна, мне больше нравится первый.

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

foreach (clsPoint CurrentPoint in ExcellentPoints) {
    if (!IsAwesome1 && 
            IsCloseThan(AwesomePoint1, CurrentPoint, CheckPoint) && 
            IsFarAndIncludePoint(AwesomePoint2, CurrentPoint, CheckPoint) && 
            (IsConnected || IsConnectedToBase1(AwesomePoint2, CurrentPoint, BasePoint1))) {
        AwesomePoint1 = CurrentPoint;
    }
    elseif (!IsAwesome2 && 
            IsCloseThan(AwesomePoint2, CurrentPoint, CheckPoint) && 
            IsFarAndIncludePoint(AwesomePoint1, CurrentPoint, CheckPoint) &&
            (IsConnected || IsConnectedToBase2(AwesomePoint2, CurrentPoint, BasePoint2))) {
        AwesomePoint2 = CurrentPoint;
    }
}
private bool IsCloseThan(clsPoint NewPoint, clsPoint CurrentPoint, clsPoint CheckPoint) {
    return CurrentPoint.DistanceToPoint(CheckPoint) < NewPoint.DistanceToPoint(CheckPoint) || NewPoint.DistanceToPoint(CheckPoint) < 1;
}
private bool IsFarAndIncludePoint(clsPoint NewPoint, clsPoint CurrentPoint, clsPoint IncludePoint) {
    return NewPoint == null || IncludePoint.isInToLine(CurrentPoint, NewPoint) && CurrentPoint.DistanceToPoint(NewPoint) > 1;
}
private bool IsConnectedToBase1(clsPoint NewPoint, clsPoint CurrentPoint, clsPoint CheckPoint) {
    clsPoint ConnectPoint1 = GetConnectPoint1(NewPoint);
    return CheckPoint.Classify(ConnectPoint1, NewPoint) == CurrentPoint.Classify(ConnectPoint1, NewPoint);
}
private bool IsConnectedToBase2(clsPoint NewPoint, clsPoint CurrentPoint, clsPoint CheckPoint) {
    clsPoint ConnectPoint1 = GetConnectPoint2(NewPoint);
    return CheckPoint.Classify(ConnectPoint2, NewPoint) == CurrentPoint.Classify(ConnectPoint2, NewPoint);
}
private clsPoint GetConnectPoint1(clsPoint Point) {
    return Point.GetConnectBeem(this)[0].Point1;
}
private clsPoint GetConnectPoint2(clsPoint Point) {
    return Point.GetConnectBeem(this)[0].Point2;
}

Теория

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