Govnokodu.Net

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

Исходный код

if (isRegKeyRequired) {
    keyItems = new XItem [4];
    valueItems = new XItem [4][];
    totalItems = 4;

    keyItems[0] = new XItem();
    keyItems[0].setLabel(TextUtils.getText(280), XItem.LABEL_CONSTRAINT_CENTER);
    values = getValueItems(countryBox_rp);
    totalItems += setValues(valueItems, values,0);
    keyItems[1] = newXItem();
    keyItems[1].setLabel(TextUtils.getText(281), XItem.LABEL_CONSTRAINT_CENTER);
    values = getValueItems(phoneNoBox_rp);
    totalItems += setValues(valueItems, values,1);
    keyItems[2] = newXItem();
    keyItems[2].setLabel(TextUtils.getText(282), XItem.LABEL_CONSTRAINT_CENTER);
    values = getValueItems(regKeyBox_rp);
    totalItems += setValues(valueItems, values, 2);
    keyItems[3] = newXItem();
    keyItems[3].setLabel(TextUtils.getText(283), XItem.LABEL_CONSTRAINT_CENTER);
    values = getValueItems(emailBox_rp);
    totalItems += setValues(valueItems, values,3);
}
else {
    keyItems = new XItem [3];
    valueItems = new XItem [3][];
    totalItems = 3;

    keyItems[0] = new XItem();
    keyItems[0].setLabel(TextUtils.getText(280), XItem.LABEL_CONSTRAINT_CENTER);
    values = getValueItems(countryBox_rp);
    totalItems += setValues(valueItems, values, 0);
    keyItems[1] = new XItem();
    keyItems[1].setLabel(TextUtils.getText(281), XItem.LABEL_CONSTRAINT_CENTER);
    values = getValueItems(phoneNoBox_rp);
    totalItems += setValues(valueItems, values,1);
    keyItems[2] = new XItem();
    keyItems[2].setLabel(TextUtils.getText(283), XItem.LABEL_CONSTRAINT_CENTER);
    values = getValueItems(emailBox_rp);
    totalItems += setValues(valueItems, values,2);
}

Анализ недостатков исходного кода

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

Вижу, что тут есть две сущности: Label и Value, сильно связанные между собой, при этом хранящиеся в двух разных массивах. При наличии такой тесной связи есть смысл эту связь выразить в явном виде путём объединения их в третью сущность LabeledValue.

Есть смысл избавиться от дуэта getValueItems — setValues, так как в целом они вдвоём выполняют в приницпе одну операцию: создают массив элементов-значений.

Вторая ветка алгоритма почти идентична первой — это плохо, ветвление должно отвечать только за различающиеся части кода, а одинаковые нужно «вынести за скобки». Тут снова дублирование реализации, но уже более высокого уровня: создания повторяющихся трёх элементов.

Переменная values всё время перезаписывается. Смысл хранящихся в ней значений хоть и похож, но не в точности тот же самый.

Имеют место «магические литералы» 280, 281 и т. п.: такие вещи необходимо реализовывать в виде именованных констант.

Как отрефакторить исходный код

Я не знаю, каков тип значений, хранящихся в переменных countryBox_rp, phoneNoBox_rp, поэтому для определённости предположу, что там строки.

class LabelledValue {
    private XItem label;
    private XItem values[];

    public LabelledValue(String values[], int labelCode, int labelAlign = XItem.LABEL_CONSTRAINT_CENTER) {
        LabelledValue(values, TextUtils.getText(labelCode), labelAlign);
    }

    public LabelledValue(String values[], String label, int labelAlign = XItem.LABEL_CONSTRAINT_CENTER) {
        this.values = newXItem[values.length];
        for (inti=0; i < values.length; i++) {
            this.values[i] = createValue(values[i]);
        }
        this.label = createLabelItem(label, labelAlign);
    }

    public int totalItems() {
        return values.length + 1;
    }

    protected XItem createValue(String value) {
        // ...
    }

    protected XItem createLabel(String label, int labelAlign) {
        XItem item=new XItem();
        item.setLabel(label, labelAlign);
        return item;
    }
}

Обрати внимание, что из конструктора, который отвечает за создание пары метка-значение, убраны ответственности за создание собственно метки и значения.

После того, как все реализации спрятаны в соответствующие методы, исходный алгоритм становится совсем простой:

List <LabelledValue> items = new List <LabelledValue>;
items.add(new LabelledValue(countryBox_rp, 280));
items.add(new LabelledValue(phoneNoBox_rp, 281));
if (isRegKeyRequired) {
    items.add(new LabelledValue(regKeyBox_rp, 282));
}
items.add(new LabelledValue(emailBox_rp, 283));

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