Комментарии 27
Без ленивых структур данных — выглядит крайне сомнительно.
теперь чтобы понять как работает код, надо еще отдельно разобраться с FartherThan и consecutive… например если в нем закрадется ошибка
FartherThan и consecutive своим названием говорят о том, что они делают на нижнем уровне абстракции, и вникать в их реализацию не нужно, если в них нет ошибки. Это как не надо вникать, что "ходить" на самом деле есть "повторять несколько раз (шаг левой ногой, шаг правой ногой)", до тех пор пока не сломал позвоночник и не начал учиться ходить заново.
Все довольно относительно. Казалось бы функция computeNumberOfBreaks тоже своим названием вполне показывает что она делает. Зачем лезть внутрь? Внутрь нужно лезть если хочешь понять как именно считаются эти остановки. Точно также мне нужно будет посмотреть внутрь ещё одной сущности, если мне нужно будет понять как все-таки считаются эти остановки (по какому пути? По карте? По прямому расстоянию? По какой геометрии?)
С другой стороны эту же проблему можно было бы решить нормальным наименованием переменных. Использовать не it1, it2, а cur, prev. Использовать auto, чтобы не перезагружать код. Записать географические точки в переменные с хорошим названием, чтобы было понятно. Вообще пример слишком мал для какого-то вывода. Нужно понимать в каком контексте будут использоваться эти функции и классы, оправданно ли перегрузка абстракциями или нет. Но мне бы такой код было бы читать приятнее.
С другой стороны эту же проблему можно было бы решить нормальным наименованием переменных. Использовать не 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 ;
}Ваше стремление сделать всё в одном месте понятнее в итоге привело к более длинному коду, который в ряд ли стал понятнее, и к ошибке в случае route.empty() == true.
по поводу пустого route справедливо. По поводу понятливости… Он точно более понятен, чем первый вариант, тут вопросов нет. Со вторым вариантом (через абстракции) все сложнее. Как я говорил, пример слишком мал, чтобы что-то утверждать наверняка. В таком виде мне сложнее парсить что такое FartherThan и consecutive (тем более, если они находятся не прямо рядом с кодом где используется, а где-нибудь в другом файле, что вполне возможно).
С другой стороны, если принять во внимание возможность модификации, то дополнительные абстракции нужны. Но скорее всего не такие.
ps. Ну а длина кода вообще не аргумент.
С другой стороны, если принять во внимание возможность модификации, то дополнительные абстракции нужны. Но скорее всего не такие.
ps. Ну а длина кода вообще не аргумент.
Да, но покрыв их один раз модульными тестами, вы сможете не переживать за их корректность во всех других местах, где они будут использоваться.
удалил
Новый код не эквивалентен старому в смысле алгоритма. Не факт, что он впишется в условия по использованной памяти.
Здесь всё упрощено для примера. Если важно потребление памяти, то можно реализовать адаптер к итератору коллекции, который при разыменовании будет возвращать смежную пару. Новый код останется почти без изменений.
Здесь — тоже упрощено:
Если бы ваше предложение было серебряной пулей, вы бы его увидели в книжках 80-х. Увы, наращивание уровней абстракции, в общем случае, ведёт лишь к увеличению кодовой базы. А это, в общем случае, не уменьшает энтропии.
А чаще всего код становится менее понятным, так как для ряда операций очень сложно придумать лаконичные названия, и 10 строчек превращаются в SelectEveryThirdTicketWithOddNumber, а если кодер плохо разбирается в теме или обладает плохой памятью, то в getTheThingJohnToldYouLastSunday.
И, всё-таки, не стоит переоценивать компилятор. Да, он это всё прожуёт, но жевать он будет дольше. На этапе отладки боем это очень неприятно сказывается на процесс.
int getRandomNumber()
{
return 4; // chosen by fair dice roll
// guaranteed to be random
}Если бы ваше предложение было серебряной пулей, вы бы его увидели в книжках 80-х. Увы, наращивание уровней абстракции, в общем случае, ведёт лишь к увеличению кодовой базы. А это, в общем случае, не уменьшает энтропии.
А чаще всего код становится менее понятным, так как для ряда операций очень сложно придумать лаконичные названия, и 10 строчек превращаются в SelectEveryThirdTicketWithOddNumber, а если кодер плохо разбирается в теме или обладает плохой памятью, то в getTheThingJohnToldYouLastSunday.
И, всё-таки, не стоит переоценивать компилятор. Да, он это всё прожуёт, но жевать он будет дольше. На этапе отладки боем это очень неприятно сказывается на процесс.
Зачем вообще шаманство с итераторами, когда можно использовать старый добрый индекс? С ним код был бы прямым и простым. Ну еще добавить DistanceTo в City. Вместо этого вы навернули неоптимальных врапперов, которые надо отдельно документировать и тестировать, в итоге читатель потратит больше времени на понимание как именно это работает.
Зачем вообще шаманство с итераторами, когда можно использовать старый добрый индекс?
Если вам завтра скажут «в новой версии не std::vector, а std::list», подход с индексами придется переделывать, причем на те же самые итераторы. Подход с итераторами при этом не сломается
Не скажут, т.к. во-первых лист не нужен, а во-вторых, интерфейс принимает вектор. Если б он принимал итераторы, как stl, это была бы другая история (но в 99 процентах ситуаций ты хочешь работать с конкретным контейнером).
с индексами больше возможности ошибки. С range-based for вероятность ошибки ещё меньше, но его не везде можно использовать (как, например, тут, где нужны 2 значения одновременно)
Мне кажется, что когда вовлечена математика, то индексы гораздо чище (что если вам нужно через один город смотреть, например? через н?). Но это на самом деле мелочи. Если вам часто надо смотреть на соседние города, то вы заведете какой-нибудь CityMap, в котором для каждого City будет метод getNeighbours() и вам не надо будет создавать какие-то пары на лету. Нужно фокусироваться на абстракциях, которые имеют смысл, а не на разделении 5 строчек кода на две функции и класс.
зачем велосипедить на индексах то, что уже реализовано, есть в стандартной библиотеке и не ограничено контейнерами с произвольным доступом?
И да, сложная формула на бумажке и её реализация в коде — разные вещи.
И да, сложная формула на бумажке и её реализация в коде — разные вещи.
Во-первых, это не велосипед. Во-вторых, я не собираюсь холиворить про индексы vs итераторы, суть вообще не в этом, а в том, что изначально был трудночитаемый код на итераторах. Его можно сделать нормальным хоть на индексах, хоть на итераторах и не «велосипедить» мутные врапперы которые копируют объекты из коллекции.
Вообще существует распространенная ошибка, когда считают что выделением методов можно всегда упростить код. Если в этом коде что то сломается, то вместо того что бы смотреть в один локальный кусок, мне нужно будет метаться между двумя кусками(я же не знаю где именно проблема).
Особенно плохо, если новый метод не выходи понятно назвать.
Так что тут всегда трейдофф. И в данном случае до рефакторинга код был проще.
Особенно плохо, если новый метод не выходи понятно назвать.
Так что тут всегда трейдофф. И в данном случае до рефакторинга код был проще.
Если это модельный пример, тогда, чтобы действительно прочувствовать силу подхода, хотелось бы увидеть и боевой пример. Если же этот пример не модельный — возникает вопрос о целесообразности создания всех этих абстракций для реализации настолько простого алгоритма.
Чем плох такой, слегка изменённый вариант решения?
Такой код прост, достаточно компактный и, наконец, эффективный.
Будет ли consecutive для любых контейнеров абстракцией нулевой стоимости?
А гибкость — нужна ли она в этом случае?
ИМХО, за исключением эффективности, какой из вариантов лучше — чистая вкусовщина.
Чем плох такой, слегка изменённый вариант решения?
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 для любых контейнеров абстракцией нулевой стоимости?
А гибкость — нужна ли она в этом случае?
ИМХО, за исключением эффективности, какой из вариантов лучше — чистая вкусовщина.
В тексте же написано, что такой код не отвечает на вопрос, что он делает, пока не просмотришь строчку за строчкой. В этом и проблема такого подхода. Такой код сложнее изменить, и он сам по себе станет ещё непонятнее, если поменять условия остановок между городами. И у вас ошибка неопределённого поведения при route.empty() == true. Чтобы мне её найти, пришлось понять весь код. При несмешивании уровней абстракций, такая ошибка не возникла бы.
Естественно, что единственный способ упростить решение сложной задачи — её декомпозиция. И я не выступаю противником декомпозиции, несмотря на предложенный вариант решения. Я лишь сомневаюсь в целесообразности такой декомпозиции в таком простом случае, ведь понятия «сложно/просто» относительные.
Остаётся consecutive. Чтож, соглашусь, он действительно упростит решение, если его функционал будет использоваться в проекте многократно, если же нет — мы меняем сложность восприятия 3-х строчек (заголовка цикла for) на сложность восприятия 1-й строчки + документации к consecutive.
По поводу ошибки — да, есть такая. Хотя для её поиска не надо понимать весь код, она в самом начале :). И такая же ошибка могла бы появиться при написании consecutive. Но, да, согласен, лишь однажды, что возвращает нас к вопросу о многократности применения.
Такой код сложнее изменить, и он сам по себе станет ещё непонятнее, если поменять условия остановок между городамиКонечно, если это условие станет сложнее, или понадобится в нескольких местах, его просто обязательно следует вынести в отдельную функцию.
Остаётся consecutive. Чтож, соглашусь, он действительно упростит решение, если его функционал будет использоваться в проекте многократно, если же нет — мы меняем сложность восприятия 3-х строчек (заголовка цикла for) на сложность восприятия 1-й строчки + документации к consecutive.
По поводу ошибки — да, есть такая. Хотя для её поиска не надо понимать весь код, она в самом начале :). И такая же ошибка могла бы появиться при написании consecutive. Но, да, согласен, лишь однажды, что возвращает нас к вопросу о многократности применения.
Такой код прост, достаточно компактный и, наконец, эффективный.
Чтобы понять, что делает этот код, нужно прочитать его полностью. Зная, что делают consecutive, FartherThan и count_if, код, их использующий, читается очень быстро. И меньше вероятность ошибки.
Я знаю, что начиная с 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. а вообще, дистанцию между двумя точками лучше оценивать свободной функцией
Автор, ты на верном пути.
Все циклы уже написаны и включены в стандартную библиотеку и Буст. Если задача сводится к такому уже написанному циклу, то лучше собрать решение из готовых кусков, чем каждый раз писать всё заново, вываливая на читателя тонны лапши и рискуя допустить ошибку.
Функциональщина и декларативщина стремительно врываются в плюсы. Boost.Iterator, Boost.Range, range-v3 не дадут соврать.
Все же вcе субъективно и не однозначно. Само наличие большого количества мелких функций в коде точно так же начинает ухудшать его читабельность как и одна мегафункция. Когда у тебя в файле, вместо 10 функций по 20 строк, 100 функций по 2 сторки, начинаются труднсти. В данном примере не видно, так как он мал сам по себе и такое преобразование, кажется, вполне логично улучшает его читабельность. Но в кодовой базе куда больше все уже не так однозначно становится. Вот, например, немножко другой взляд на вещи:
habrahabr.ru/company/nixsolutions/blog/341034
habrahabr.ru/company/nixsolutions/blog/341034
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Супер-выразительный код с привлечением уровней абстракций