Revert "All necessary files were removed" by senyacherenkov · Pull Request #1 · senyacherenkov/lesson_3_homework

senyacherenkov · 2018-08-15T16:18:33Z

This reverts commit df715fe.

rfrolov

Здравствуйте, Арсений.
Домашнее задание удовлетворяет ТЗ. Текст понятен, читается легко. Очень хорошо что реализованы тесты.
По реализации есть замечания/предложения, которые отметил в коде.

rfrolov · 2018-08-16T15:35:22Z

filter_helper.cpp

+
+    rIpStr.append(input.at(0));    
+
+    for(std::vector<std::string>::const_iterator ip_part = input.cbegin() + 1; ip_part != input.cend(); ++ip_part)


Красивее сделать объявление ip_part через auto:
for(auto ip_part = input.cbegin() + 1; ip_part != input.cend(); ++ip_part)

или через range for:
for (const auto& ip_part: input)

rfrolov · 2018-08-16T16:06:47Z

filter_helper.h

+namespace filtering {
+
+    using addressList = std::vector<std::vector<std::string>>;
+


По тексту много где повторяется std::vectorstd::string
Предлагаю ввести алиас через using этого типа.

Это общепринято. Если есть какой-то алиас на типы из STL, то при чтении кода можно увидеть какой-нибудь vector и придется идти и разбираться что это за вектор, стандартный или нет.

rfrolov · 2018-08-16T16:10:32Z

filter_helper.cpp

+
+void showList(const addressList& input)
+{
+    for (auto & ip: input)


Здесь и далее правильнее и безопаснее пользоваться квалификатором const.
for (const auto & ip: input).

rfrolov · 2018-08-16T16:15:35Z

filter_helper.cpp

+        }
+
+        std::string suitableIp = getIp(ip);
+        rVecStr.push_back(suitableIp);


Здесь и далее код:

std::string suitableIp = getIp(ip); rVecStr.push_back(suitableIp);

можно написать более оптимально:
rVecStr.emplace_back(getIp(ip));

rfrolov · 2018-08-16T16:19:23Z

filter_helper.cpp

+
+void lexicographRevSort(addressList& input)
+{
+    std::sort(input.rbegin(), input.rend(),[](const std::vector<std::string> & lhs,


Самописную лямбду можно заменить на std::greater<>.

Yukigaru

На мой взгляд, хранение ip адреса в строках - плохая идея. Стоит рассмотреть хранение обычных чисел.
Кроме этой фундаментальной проблемы, замечаний немного.

Yukigaru · 2018-09-04T09:12:39Z

filter_helper.h

+namespace filtering {
+
+    using addressList = std::vector<std::vector<std::string>>;
+


Это общепринято. Если есть какой-то алиас на типы из STL, то при чтении кода можно увидеть какой-нибудь vector и придется идти и разбираться что это за вектор, стандартный или нет.

Yukigaru · 2018-09-04T09:14:10Z

filter_helper.cpp

+
+    rIpStr.append(input.at(0));    
+
+    for(std::vector<std::string>::const_iterator ip_part = input.cbegin() + 1; ip_part != input.cend(); ++ip_part)


или через range for:
for (const auto& ip_part: input)

Yukigaru · 2018-09-04T18:47:59Z

filter_helper.cpp

+        bool isByteFounded = false;
+        for (auto& ip_part: ip)
+        {
+            if (std::strcmp(ip_part.c_str(), byte1) == 0) {


хранить и сравнивать строки очень неэффективно, когда речь идёт о простых числах.
стоило рассмотреть хранение октетов в integral типе

с другой стороны, если все же сравнивается std::string и const char *, то можно сравнить напрямую, без strcmp

Yukigaru · 2018-09-04T18:55:58Z

filter_helper.cpp

+{
+    if(lhs.size() == rhs.size())
+    {
+        return std::lexicographical_compare(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());


исп-е lexicompare - здорово

но что будет, если сравнить
1.111.1.1 и 1.11.111.1?

Yukigaru · 2018-09-04T19:03:19Z

ip_filter.cpp

+#include <algorithm>
+#include <tuple>
+#include <stdexcept>
+#include "filter_helper.h"


лишние инклуды

PetrLiutik · 2018-09-17T08:32:44Z

Оценка ревью Романа.
С большинством замечаний/предложений Романа (rfrolov) согласен. Конечно можно было бы еще сделать замечание по использованию чисел для хранения и обработки ip-адресов вместо строк.

Revert "All necessary files were removed"

a0ecde8

This reverts commit df715fe.

rfrolov reviewed Aug 16, 2018

View reviewed changes

Yukigaru suggested changes Sep 4, 2018

View reviewed changes


		rIpStr.append(input.at(0));

		for(std::vector<std::string>::const_iterator ip_part = input.cbegin() + 1; ip_part != input.cend(); ++ip_part)

		namespace filtering {

		using addressList = std::vector<std::vector<std::string>>;

Conversation

senyacherenkov commented Aug 15, 2018

Uh oh!

rfrolov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yukigaru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetrLiutik commented Sep 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants