search

OlegWorld / ip_filter

0 stars 0 forks source link

Review Anton Illarionov #2

Open antik9 opened 6 years ago

antik9 commented 6 years ago

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

  2. Есть тесты. есть проверка с stdout, это плюс. Для более читабельного вида эту строку https://github.com/OlegWorld/ip_filter/blob/master/filter_tests.cpp#L110 можно было бы разбить на запись вида: "192.168.0.1\n" "192.165.6.4\n" Кажется так гораздо удобнее читать, особенно находить опечатки. Но по самим тестам все ок.

  3. В этой строке происходит три операции. Опять же для сохранения читабельности кода, можно было бы разбить, например, на две операции. Хотя строка не очень длинная, но приходить вникать в последовательность действий. Not critical. https://github.com/OlegWorld/ip_filter/blob/master/ip_filter.cpp#L17

  4. Выброс исключения ок, но само условие выглядит довольно необычно, так как выбрасывается только на третьем шаге цикла. Плюс опираться на сравнение std::string::npos с std::string::npos + 1, которое хранится в переменной start на последнем шаге цикла как-то довольно костыльно) https://github.com/OlegWorld/ip_filter/blob/master/ip_filter.cpp#L21 В целом логика солбюдена, но не хватает линейности что ли...

  5. Что касается в принципе логики вывода фильтрованных ip, то кажется, что это скорее бы хотелось сделать in-place, а не создавать для этого отдельные структуры, которые хранят фильтрованные данные. Допустим, нам бы хотелось это исполнять в 8 потоков и размер исходного вектора был бы несколько сотен метров. Тогда потребление по памяти для параллельной фильтрации съело бы несколько гигов оперативки, в худшем случае еще 8 таких векторов. Это вообще не кейс данной задачи, но в принципе про in-place остается актуально, тем более, что из твоего ревью я и сам заметил, что, забыв передать по ссылке, я вообще не пожалел нисколько памяти для программы)

Общие замечания: В целом, когда начинаешь читать чей-то код, сразу бросается в глаза какие-то вещи, которые могут остаться незамеченными в своем же коде. Названия переменных, декомпозиция сложных операций... И именно на это я обратил внимание. Но по своей сути, мне понравился код кроме одного места с условием выброса исключения. Так держать! Возможно еще, будет возможность провести ревью на каком-то из следующих домашних заданий.

OlegWorld commented 6 years ago

Спасибо за ревью и ценные замечания. Постараюсь учесть в будущем.