Супер-выразительный код с привлечением уровней абстракций / forpes.ru

Главная
Супер-выразительный код с привлечением уровней абстракций

Супер-выразительный код с привлечением уровней абстракций +6

26.10.2017 08:46

sergio_nsk 27 5400 Источник

Предлагаю вашему вниманию перевод статьи Super expressive code by Raising Levels of Abstraction

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

Проблема

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

Пользователь нашего приложения планирует поездку через несколько городов в стране.

Он едет сразу из одного города в другой без остановки, если они достаточно близкие (скажем, на расстоянии до 100 км), иначе он делает ровно одну остановку между городами.

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

Наша задача рассчитать необходимое количество остановок, экономя время на поезду.

Приложение уже имеет класс City, которым описывается город на маршруте. City предоставляет свои географические атрибуты, среди которых есть местоположение, реализованное классом Location. Объект класса Location может вычислять длину маршрута до любого другого объекта Location на карте.

class Location
{
public:
    double distanceTo(const Location &other) const;
    ...
};

class GeographicalAttributes
{
public:
    Location getLocation() const;
    ...
};

class City
{
public:
    const GeographicalAttributes &getGeographicalAttributes() const;
    ...
};

Теперь предлагается реализация вычисления необходимого количества остановок, которые придётся совершить пользователю:

#include <vector>

int computeNumberOfBreaks(const std::vector<City> &route)
{
    static const double MaxDistance = 100;

    int nbBreaks = 0;
    for (std::vector<City>::const_iterator it1 = route.begin(), it2 = route.end();
         it1 != route.end();
         it2 = it1, ++it1)
    {
        if (it2 != route.end())
        {
            if(it1->getGeographicalAttributes().getLocation().distanceTo(
                it2->getGeographicalAttributes().getLocation()) > MaxDistance)
            {
                ++nbBreaks;
            }
        }
    }
    return nbBreaks;
}

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

Давайте поработаем над этим куском кода и превратим его в ваш актив.

Создаём ясный код

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

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

Чтобы так сделать, мы можем применить следующую технику:

Определить, какие вещи делает код, и заменить их осмысленными метками

Это приведёт к значительному улучшению ясности кода.

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

Начнём с логики цикла.

for (std::vector<City>::const_iterator it1 = route.begin(), it2 = route.end();
     it1 != route.end();
     it2 = it1, ++it1)
{
   if (it2 != route.end())
   {

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

Мы сразу не могли сказать, что этот код понятный. Но после описания трюка мы можем определённо сказать, что он делает: он производит манипуляции с двумя смежными элементами за раз.

Давайте теперь рассмотрим другой кусок кода в условии:

it1->getGeographicalAttributes().getLocation().distanceTo(
    it2->getGeographicalAttributes().getLocation()) > MaxDistance

Сам по себе этот код довольно легко проанализировать и понять, что он делает. Он определяет, что между двух городов расстояние больше, чем MaxDistance.

И наконец, проанализируем остаток кода, переменную nbBreaks:

int nbBreaks = 0;
for (...)
{
       if(...)
       {
           ++nbBreaks;
       }
}
return nbBreaks;

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

В итоге получаем такие метки, которые описывают, что делает функция:

Манипуляции смежными элементами,
Определение, что два города находятся на расстоянии большем MaxDistance,
Число, сколько раз условие было удовлетворено.

После того, как анализ сделан, остался только один шаг до превращения неясного кода в выразительный.

Методика заключается в назначении метки на каждую сущность, реализуемой кодом, и заменой соответствующего кода этой меткой. Здесь мы проделаем следующее:

Для манипуляции смежными элементами мы можем создать компонент, который мы назовём consecutive, трансформирующим коллекцию элементов в коллекцию пар элементов, каждая пара будет иметь элемент из изначальной коллекции и следующий за ним элемент. Если маршрут route содержит {A, B, C, D, E}, consecutive(route) будет создавать {(A, B), (B, C), (C, D), (D, E)}. Подобный адаптер, создающий пары смежных элементов, недавно был добавлен под именем sliding в популярную библиотеку range-v3.
Для определения превышения расстояния MaxDistance между двумя городами мы можем использовать функциональный объект (functor), назовём его FartherThan. Я знаю, что начиная с C++11 функторы в основном можно заменить лямбда-функциями, но здесь нам нужно дать имя действию. Чтобы сделать это элегантно с помощью лямбда-функции, нужно немного больше работы, о чём я расскажу в отдельном посте.
```
class FartherThan
{
public:
   explicit FartherThan(double distance) : m_distance(distance) {}
   bool operator()(const std::pair<City, City> &cities)
   {
          return cities.first.getGeographicalAttributes().getLocation().distanceTo(
                 cities.second.getGeographicalAttributes().getLocation()) > m_distance;
   }
private:
   double m_distance;
};
```
Для вычисления числа, сколько раз условие было удовлетворено, мы можем использовать алгоритм count_if из STL.

Вот, что в итоге получается после замены кода метками:

int computeNumberOfBreaks(const std::vector<City>& route)
{
    static const double MaxDistance = 100;

    return count_if(consecutive(route), FartherThan(MaxDistance));
}

(Примечание: count_if из STL принимает два итератора от начала и до конца коллекции. Здесь используется обёртка count_if над std::count_if, которая передаёт в стандартный std::count_if до C++17 начало и конец коллекции.)

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

Эта техника может применяться ко многим частям неясного кода, чтобы преобразовать его в ясный. Она так же может быть применена для других языков программирования. В следующий раз, когда вы наткнётесь на неясный код, который вам захочется отрефакторить, попробуйте идентифицировать вещи, который делает код, и присвойте им имена. Вы будет поражены достигнутым результатом.

Комментарии (27)

aamonster
26.10.2017 13:01
#10492324
-1
Без ленивых структур данных — выглядит крайне сомнительно.

BriGaDir_89
26.10.2017 13:37
#10492376
+2
теперь чтобы понять как работает код, надо еще отдельно разобраться с FartherThan и consecutive… например если в нем закрадется ошибка
1. sergio_nsk Автор
  26.10.2017 13:49
  #10492394
  FartherThan и consecutive своим названием говорят о том, что они делают на нижнем уровне абстракции, и вникать в их реализацию не нужно, если в них нет ошибки. Это как не надо вникать, что "ходить" на самом деле есть "повторять несколько раз (шаг левой ногой, шаг правой ногой)", до тех пор пока не сломал позвоночник и не начал учиться ходить заново.
  1. myxo
    26.10.2017 16:04
    #10492638
    +1
    Все довольно относительно. Казалось бы функция computeNumberOfBreaks тоже своим названием вполне показывает что она делает. Зачем лезть внутрь? Внутрь нужно лезть если хочешь понять как именно считаются эти остановки. Точно также мне нужно будет посмотреть внутрь ещё одной сущности, если мне нужно будет понять как все-таки считаются эти остановки (по какому пути? По карте? По прямому расстоянию? По какой геометрии?)
    
    С другой стороны эту же проблему можно было бы решить нормальным наименованием переменных. Использовать не it1, it2, а cur, prev. Использовать auto, чтобы не перезагружать код. Записать географические точки в переменные с хорошим названием, чтобы было понятно. Вообще пример слишком мал для какого-то вывода. Нужно понимать в каком контексте будут использоваться эти функции и классы, оправданно ли перегрузка абстракциями или нет. Но мне бы такой код было бы читать приятнее.
    
    код
    int computeNumberOfBreaks(const std::vector<City> &route) { static const double MaxDistance = 100; int breaks_num = 0; auto cur = route.cbegin() auto prev = cur++; for ( ;cur != route.cend(); prev++, cur++) { auto cur_location = cur->getGeographicalAttributes().getLocation(); auto prev_location = prev->getGeographicalAttributes().getLocation(); if(cur_location.DistanceTo(prev_location) > MaxDistance) { breaks_num++; } } return breaks_num ; }
    
    sergio_nsk Автор
    27.10.2017 07:03
    #10493292
    Ваше стремление сделать всё в одном месте понятнее в итоге привело к более длинному коду, который в ряд ли стал понятнее, и к ошибке в случае route.empty() == true.
    
    myxo
    27.10.2017 12:16
    #10493680
    по поводу пустого route справедливо. По поводу понятливости… Он точно более понятен, чем первый вариант, тут вопросов нет. Со вторым вариантом (через абстракции) все сложнее. Как я говорил, пример слишком мал, чтобы что-то утверждать наверняка. В таком виде мне сложнее парсить что такое FartherThan и consecutive (тем более, если они находятся не прямо рядом с кодом где используется, а где-нибудь в другом файле, что вполне возможно).
    
    С другой стороны, если принять во внимание возможность модификации, то дополнительные абстракции нужны. Но скорее всего не такие.
    
    ps. Ну а длина кода вообще не аргумент.
1. pankraty
  26.10.2017 13:53
  #10492402
  Да, но покрыв их один раз модульными тестами, вы сможете не переживать за их корректность во всех других местах, где они будут использоваться.

sergio_nsk Автор
26.10.2017 13:48
#10492390
удалил

pol1234
26.10.2017 14:47
#10492506
Новый код не эквивалентен старому в смысле алгоритма. Не факт, что он впишется в условия по использованной памяти.
1. sergio_nsk Автор
  26.10.2017 14:50
  #10492512
  Здесь всё упрощено для примера. Если важно потребление памяти, то можно реализовать адаптер к итератору коллекции, который при разыменовании будет возвращать смежную пару. Новый код останется почти без изменений.
  1. iCpu
    27.10.2017 08:42
    #10493346
    Здесь — тоже упрощено:
    
    int getRandomNumber() { return 4; // chosen by fair dice roll // guaranteed to be random }
    
    Если бы ваше предложение было серебряной пулей, вы бы его увидели в книжках 80-х. Увы, наращивание уровней абстракции, в общем случае, ведёт лишь к увеличению кодовой базы. А это, в общем случае, не уменьшает энтропии.
    А чаще всего код становится менее понятным, так как для ряда операций очень сложно придумать лаконичные названия, и 10 строчек превращаются в SelectEveryThirdTicketWithOddNumber, а если кодер плохо разбирается в теме или обладает плохой памятью, то в getTheThingJohnToldYouLastSunday.
    И, всё-таки, не стоит переоценивать компилятор. Да, он это всё прожуёт, но жевать он будет дольше. На этапе отладки боем это очень неприятно сказывается на процесс.

daiver19
26.10.2017 21:32
#10493004
Зачем вообще шаманство с итераторами, когда можно использовать старый добрый индекс? С ним код был бы прямым и простым. Ну еще добавить DistanceTo в City. Вместо этого вы навернули неоптимальных врапперов, которые надо отдельно документировать и тестировать, в итоге читатель потратит больше времени на понимание как именно это работает.
1. Antervis
  27.10.2017 11:20
  #10493570
  Зачем вообще шаманство с итераторами, когда можно использовать старый добрый индекс?
  
  Если вам завтра скажут «в новой версии не std::vector, а std::list», подход с индексами придется переделывать, причем на те же самые итераторы. Подход с итераторами при этом не сломается
  1. daiver19
    27.10.2017 17:15
    #10494314
    Не скажут, т.к. во-первых лист не нужен, а во-вторых, интерфейс принимает вектор. Если б он принимал итераторы, как stl, это была бы другая история (но в 99 процентах ситуаций ты хочешь работать с конкретным контейнером).
    
    myxo
    27.10.2017 19:45
    #10494826
    с индексами больше возможности ошибки. С range-based for вероятность ошибки ещё меньше, но его не везде можно использовать (как, например, тут, где нужны 2 значения одновременно)
    
    daiver19
    27.10.2017 20:07
    #10494864
    Мне кажется, что когда вовлечена математика, то индексы гораздо чище (что если вам нужно через один город смотреть, например? через н?). Но это на самом деле мелочи. Если вам часто надо смотреть на соседние города, то вы заведете какой-нибудь CityMap, в котором для каждого City будет метод getNeighbours() и вам не надо будет создавать какие-то пары на лету. Нужно фокусироваться на абстракциях, которые имеют смысл, а не на разделении 5 строчек кода на две функции и класс.
    
    Antervis
    27.10.2017 22:15
    #10495024
    зачем велосипедить на индексах то, что уже реализовано, есть в стандартной библиотеке и не ограничено контейнерами с произвольным доступом?
    
    И да, сложная формула на бумажке и её реализация в коде — разные вещи.
    
    daiver19
    27.10.2017 22:37
    #10495062
    Во-первых, это не велосипед. Во-вторых, я не собираюсь холиворить про индексы vs итераторы, суть вообще не в этом, а в том, что изначально был трудночитаемый код на итераторах. Его можно сделать нормальным хоть на индексах, хоть на итераторах и не «велосипедить» мутные врапперы которые копируют объекты из коллекции.

rumatavz
27.10.2017 00:47
#10493148
Вообще существует распространенная ошибка, когда считают что выделением методов можно всегда упростить код. Если в этом коде что то сломается, то вместо того что бы смотреть в один локальный кусок, мне нужно будет метаться между двумя кусками(я же не знаю где именно проблема).
Особенно плохо, если новый метод не выходи понятно назвать.
Так что тут всегда трейдофф. И в данном случае до рефакторинга код был проще.

AntonGilyov
27.10.2017 07:03
#10493294
Если это модельный пример, тогда, чтобы действительно прочувствовать силу подхода, хотелось бы увидеть и боевой пример. Если же этот пример не модельный — возникает вопрос о целесообразности создания всех этих абстракций для реализации настолько простого алгоритма.
Чем плох такой, слегка изменённый вариант решения?
```
int computeNumberOfBreaks(const std::vector<City> &route)
{
    static const double MaxDistance = 100;

    int nbBreaks = 0;
    for (auto cur = route.begin(), next = cur + 1;
        next != route.end();
        cur = next, ++next)
    {
        auto curLoc = cur->getGeographicalLocation().getLocation();
        auto nextLoc = next->getGeographicalLocation().getLocation();

        if (curLoc.distanceTo(nextLoc) > MaxDistance)
        {
            ++nbBreaks;
        }
    }
    return nbBreaks;
}
```
Такой код прост, достаточно компактный и, наконец, эффективный.
Будет ли consecutive для любых контейнеров абстракцией нулевой стоимости?
А гибкость — нужна ли она в этом случае?
ИМХО, за исключением эффективности, какой из вариантов лучше — чистая вкусовщина.
1. sergio_nsk Автор
  27.10.2017 07:10
  #10493300
  В тексте же написано, что такой код не отвечает на вопрос, что он делает, пока не просмотришь строчку за строчкой. В этом и проблема такого подхода. Такой код сложнее изменить, и он сам по себе станет ещё непонятнее, если поменять условия остановок между городами. И у вас ошибка неопределённого поведения при route.empty() == true. Чтобы мне её найти, пришлось понять весь код. При несмешивании уровней абстракций, такая ошибка не возникла бы.
  1. AntonGilyov
    27.10.2017 12:17
    #10493684
    Естественно, что единственный способ упростить решение сложной задачи — её декомпозиция. И я не выступаю противником декомпозиции, несмотря на предложенный вариант решения. Я лишь сомневаюсь в целесообразности такой декомпозиции в таком простом случае, ведь понятия «сложно/просто» относительные.
    Такой код сложнее изменить, и он сам по себе станет ещё непонятнее, если поменять условия остановок между городами
    Конечно, если это условие станет сложнее, или понадобится в нескольких местах, его просто обязательно следует вынести в отдельную функцию.
    Остаётся consecutive. Чтож, соглашусь, он действительно упростит решение, если его функционал будет использоваться в проекте многократно, если же нет — мы меняем сложность восприятия 3-х строчек (заголовка цикла for) на сложность восприятия 1-й строчки + документации к consecutive.
    
    По поводу ошибки — да, есть такая. Хотя для её поиска не надо понимать весь код, она в самом начале :). И такая же ошибка могла бы появиться при написании consecutive. Но, да, согласен, лишь однажды, что возвращает нас к вопросу о многократности применения.
1. Antervis
  27.10.2017 11:31
  #10493602
  Такой код прост, достаточно компактный и, наконец, эффективный.
  
  Чтобы понять, что делает этот код, нужно прочитать его полностью. Зная, что делают consecutive, FartherThan и count_if, код, их использующий, читается очень быстро. И меньше вероятность ошибки.

Antervis
27.10.2017 11:13
#10493564
Я знаю, что начиная с C++11 функторы в основном можно заменить лямбда-функциями, но здесь нам нужно дать имя действию. Чтобы сделать это элегантно с помощью лямбда-функции, нужно немного больше работы, о чём я расскажу в отдельном посте.
```
int computeNumberOfBreaks(const std::vector<City> &route)
{
    double MaxDistance = 100;

    auto fartherThan = [=](auto &cities) {
        auto & [from, to] = cities;
        return from.getGeographicalAttributes().getLocation().distanceTo(
            to.getGeographicalAttributes().getLocation()) > MaxDistance;
    };

    return count_if(consecutive(route), FartherThan(MaxDistance));
}
```
Итого мы дали имя действию (fartherThan) и не вводили новый класс. Возможно, «элегантные лямбды» это и сложно, но уж точно проще, чем «элегантные классы для функциональных объектов»

P.S. а вообще, дистанцию между двумя точками лучше оценивать свободной функцией

izvolov
27.10.2017 13:16
#10493796
Автор, ты на верном пути.

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

Функциональщина и декларативщина стремительно врываются в плюсы. Boost.Iterator, Boost.Range, range-v3 не дадут соврать.

Leg3nd
27.10.2017 13:23
#10493812
Все же вcе субъективно и не однозначно. Само наличие большого количества мелких функций в коде точно так же начинает ухудшать его читабельность как и одна мегафункция. Когда у тебя в файле, вместо 10 функций по 20 строк, 100 функций по 2 сторки, начинаются труднсти. В данном примере не видно, так как он мал сам по себе и такое преобразование, кажется, вполне логично улучшает его читабельность. Но в кодовой базе куда больше все уже не так однозначно становится. Вот, например, немножко другой взляд на вещи:
habrahabr.ru/company/nixsolutions/blog/341034
1. Antervis
  27.10.2017 13:52
  #10493870
  идея не в том, чтобы разбить большую функцию на три маленьких, а в том, чтобы упростить большую функцию используя несколько ~~псевдо~~стандартных

Супер-выразительный код с привлечением уровней абстракций +6

Проблема

Создаём ясный код

Комментарии (27)

sergio_nsk Автор

sergio_nsk Автор

sergio_nsk Автор

sergio_nsk Автор

sergio_nsk Автор