Пример рефакторинга неудачного устранения дублирования кода

Исходный код

for (i = pdir ? old_end_y : old_end_x; i != pdir ? (sel_end_y + gstep) : (sel_end_x + gstep); i++)
{
  if (cells[pdir ? sel_x : i][pdir ? i : sel_y])
    freemem(cells[pdir ? sel_x : i][pdir ? i : sel_y]);
  if (is_arith)
  {
    cells[pdir ? sel_x : i][pdir ? i : sel_y] = ftoa(arith_first);
    arith_first += arith_step;
  }
  else
  {
  if (cells[sel_x][sel_y])
  {
    cells[pdir ? sel_x : i][pdir ? i : sel_y] = (char*)allocmem(strlen(cells[sel_x][sel_y]) + 1);
    strcpy(cells[pdir ? sel_x : i][pdir ? i : sel_y], cells[sel_x][sel_y]);
  }
}

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

Из-за обилия ветвлений по флажку pdir алгоритм вышел за грань читаемости.

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

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

// pdir == true
for (i = old_end_y; i != sel_end_y + gstep; i++)
{
  if (cells[sel_x][i]) freemem(cells[sel_x][i]);
  if (is_arith) {
    cells[sel_x][i] = ftoa(arith_first);
    arith_first += arith_step;
  }
  else {
    if (cells[sel_x][sel_y]) {
        cells[sel_x][i] = (char*)allocmem(strlen(cells[sel_x][sel_y]) + 1);
        strcpy(cells[sel_x][i], cells[sel_x][sel_y]);
    }
  }

// pdir == false
for (i = old_end_x; i != sel_end_x + gstep; i++)
{
  if (cells[i][sel_y]) freemem(cells[i][sel_y]);
  if (is_arith) {
    cells[i][sel_y] = ftoa(arith_first);
    arith_first += arith_step;
  }
  else {
    if (cells[sel_x][sel_y]) {
        cells[i][sel_y] = (char*)allocmem(strlen(cells[sel_x][sel_y]) + 1);
        strcpy(cells[i][sel_y], cells[sel_x][sel_y]);
    }
  }

Не очень понятно, зачем хранить в матрице строковые представления вещественных чисел, но ладно.

Теперь сравнивая два варианта кода вижу, что дублированием является не весь код, а только его структура. В каждой итерации цикла задействованы всего две клетки: cells[sel_x][i] либо cells[i][sel_y] и cells[sel_x][sel_y], и вот ради выбора между первыми двумя и был затеян весь сыр-бор с pdir.

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

void set_cell(char** to_cell, char* from_cell, is_arith, arith_first)
{
  if (*to_cell) freemem(*to_cell);
  if (is_arith) {
    *to_cell = ftoa(arith_first);
  }
  else {
    if (from_cell) {
        *to_cell = (char*)allocmem(strlen(from_cell) + 1);
        strcpy(*to_cell, from_cell);
    }
  }
}

Исходный цикл примет следующий вид:

if (pdir) {
  for (i = old_end_y; i != sel_end_y + gstep; i++) {
    set_cell(&cells[sel_x][i], cells[sel_x][sel_y], is_arith, arith_first);
    if (is_arith) arith_first += arith_step;
    // ...
  }
}
else {
  for (i = old_end_x; i != sel_end_x + gstep; i++) {
    set_cell(&cells[i][sel_y], cells[sel_x][sel_y], is_arith, arith_first);
    if (is_arith) arith_first += arith_step;
    // ...
  }
}

Небольшое «дублирование» здесь не является недостатком, а наоборот, улучшает читаемость и модифицируемость кода.

С помощью извлечения функции я попутно разъединил два разных знания: как организовать цикл по заданной координате и что делать с ячейкой на каждой итерации.

Govnokodu.Net

Пример рефакторинга неудачного устранения дублирования кода

Исходный код

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

Теория