Govnokodu.Net

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

Исходный код

if (typ == 4)	// Это ножницы
if (mirror == 0)
        Size_N = " " + (beems[1].LenghtE - (int)dd[0]["value"]).ToString() + "0";
    else
        Size_N = " " + (mirror - (beems[1].LenghtE - (int)dd[0]["value"])).ToString() + "0";
else
{			// Это механизм
    if (typ == 0 || typ == 2 || typ == 3)
    {
        if (mirror == 0)
            Size_M = (((int)tRow["len"] / 2 + (beems[0].LenghtE / 2 - (int)dd[0]["value"]))).ToString() + "0";
        else
            Size_M = (mirror - ((int)tRow["len"] / 2 + (beems[0].LenghtE / 2 - (int)dd[0]["value"]))).ToString() + "0";
    }
    else
        if (mirror == 0)
            Size_M = (beems[0].LenghtE - (int)dd[0]["value"]).ToString() + "0";
        else
            Size_M = (mirror - (beems[0].LenghtE - (int)dd[0]["value"])).ToString() + "0";

    if (typ == 0 || typ == 2 || typ == 3)
    {
        if (mirror == 0)
            Size_M += " " + (beems[0].LenghtE - (int)dd[0]["value"] * 2).ToString() + "0";
        else
            Size_M += " " + (mirror - (beems[0].LenghtE - (int)dd[0]["value"] * 2)).ToString() + "0";
    }
    else
        Size_M += " 0";
}

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

Первое, что бросается в глаза — что ничего не понятно и всё смешано в кучу.

Непонятен смысл условий typ == 4, mirror == 0, typ == 0 || typ == 2 || typ == 3. Непонятен смысл величин dd[0] и dd[0]["value"], beems[0], да и сами переменные mirror, Size_N, Size_M не очень понятны. Непонятен смысл вычислений, а особенно лихорадочных преобразований величин в целое число и в строку.

В такой куче неизбежно дублирование и смешение ответственностей.

Как улучшить исходный код

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

Чтобы пояснить смысл операций, их реализацию следует разобрать на отдельные небольшие методы, отделив вычисления от преобразования в строку — применяя рефакторинг «ивлечение метода».

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

На мой вкус проверку mirror == 0 лучше сделать на самом верхнем уровне.

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

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

if (hasMirror()) {
    if (isScissors()) {
        Size_N = scissorsBeamLength();
    }
    else if (isMechanism()) {
        Size_M_W = mechanismBeamWidth();
        Size_M_H = mechanismBeamHeight();
    }
    else {
        Size_M_W = paperBeamWidth();
        Size_M_H = 0;
    }
}
else {
    if (isScissors()) {
        Size_N = scissorsBeamLengthMirrored();
    }
    else if (isMechanism()) {
        Size_M_W = mechanismBeamWidthMirrored();
        Size_M_H = mechanismBeamHeightMirrored();
    }
    else {
        Size_M_W = paperBeamWidthMirrored();
        Size_M_H = 0;
    }
}

Форматирование вычисленных величин необходимо выполнять где-то в отдельном месте.

Дальше следует проанализировать все остальные связанные алгоритмы: если в них часто повторяются проверки hasMirror(), isScissors(), isMechanism() — стоит задуматься о применении рефакторинга «замена условного оператора на полиморфизм».

private int scissorsBeamLengthMirrored() {
    return mirror - scissorsBeamLength();
}
private int mechanismBeamWidthMirrored() {
    return mirror - mechanismBeamWidth();
}
private int mechanismBeamHeightMirrored() {
    return mirror - mechanismMeamHeight();
}
private int paperBeamWidthMirrored() {
    return mirror - paperBeamWidth();
}

private int scissorsBeamLength() {
    return redBeamLength() - whiteTerm();
}
private int mechanismBeamWidth() {
    return greyTerm() + (greenBeamLength() / 2 - whiteTerm());
}
private int mechanismBeamHeight() {
    return greenBeamLength() - whiteTerm() * 2;
}
private int paperBeamWidth() {
    return greenBeamLength() - whiteTerm();
}

private int whiteTerm() {
    return (int)dd[0]["value"];
}
private int greyTerm() {
    return (int)tRow["len"] / 2;
}
private int redBeamLength() {
    return beems[1].LenghtE;
}
private int greenBeamLength() {
    return beems[0].LenghtE;
}

public bool isScissors() {
    return typ == 4;
}
public bool isMechanism() {
    return typ == 0 || typ == 2 || typ == 3;
}
public bool hasMirror() {
    return mirror != 0;
}

Разумеется, настоящая реализация методов типа whiteTerm(), greyTerm(), scissorsBeamLength() и прочих не обязана быть именно такой, их извлечение продемонстрировано просто как подход к снижению количества ответственностей на участке кода.

Теория

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