Refactoring VS Здравый смысл - C/C++. Прикладные вопросы

Добрый день, уважаемые форумчане!

Пролог
В далёкой-далёкой галактике проникся важностью рефакторинга.
На планете Сириус-12 на прошлом месте работы достался проект "вырви-глаз".

Очень умные ребята класса "человек-разумный" написали очень сложночитаемый код и передали нашей команде. Сами пошли вперед, на новый проект компании.
К примеру, использовалась линия отсечки длины символов 60-70, что весь код вытягивался в трубочку вниз капая сгущенкой в 1000ный номер строки.
Использовались сложные названия переменных, сущностей.
Из-за сверхсекретной мотивации в некоторых местах куча классов объявлялись в одном файле.
И т.п.

Мой руководитель, к которому я пришел, был архитектором разряда "межгалактический эксперт" по созданию телекоммуникационного оборудования, работающего 24/7 во всех открытых и заселенных системах.
С опытом С++. Также хорошо погружен в питон, си-шарп. И, наверное, во много чего еще.

Завязка
Он уделял много внимания простоте кода. Чтобы код становился всё более читаемым.
Так, за много итераций рефакторинга в процессе работы с проектом, получалось лучше вникать в очередном модифицируемый модуль — рефакторить и улучшать читаемость.

Попутно работая с питоном я проникался идеями созданием читаемого кода. Так, чтобы { не мешала читать — правее её за объявление, куда уже никто не смотрит.
Что если можно поставить auto — ставить auto.
Что если нужен простой класс таймер — сделать простой класс таймер, а не супер-шаблоидный класс с кучами кейсов и сверхфункциональным api.

Развитие действия
И вот на следующем месте работы оказалось нужно улучшить конфиг, чтение из него и добавление функционала далее.

void Config::load(xml_wrap::NodeConst const & root_node)
{
    // Call base class first.
    ipc::SimpleConfig::load(root_node("ipc"));

    auto const micex_node = root_node("micex");
    {
        auto const connection_node = micex_node("connection");
        m_asts_connection_attempts       = connection_node.attribute<unsigned int>("attempts", "6");
        m_asts_connection_attempt_period = std::chrono::seconds(connection_node.attribute<unsigned int>("attempt-period", "300"));

        std::string asts_connection_params;
        auto const param_nodes = connection_node.children("param");
        for(auto const & param_node : param_nodes)
        {
            asts_connection_params += param_node.attribute<std::string>("name");
            asts_connection_params += '=';
            asts_connection_params += param_node.attribute<std::string>("value");
            asts_connection_params += "\r\n";
        }
        m_asts_connection_params = std::move(asts_connection_params);
    }
    {
        std::vector<subscription_t>                     asts_subscriptions;
        std::unordered_map<std::string, std::size_t>    asts_subscriptions_index;
        auto const table_nodes = micex_node("tables").children("table");
        for(auto const & table_node : table_nodes)
        {
            subscription_t subscription; // Таблица_имя, режим вычитки, ...
            subscription.m_table = table_node.attribute<std::string>("micex-name");
            if(!asts_subscriptions_index.emplace(subscription.m_table, asts_subscriptions.size()).second)
                // TODO: duplicated table warning.
                continue;
            subscription.m_snapshot = table_node.attribute<bool>("load-complete" , "true" );
            subscription.m_updates  = table_node.attribute<bool>("enable-refresh", "false");
            subscription.m_publish  = table_node.attribute<bool>("enable-publish", "true" );
            asts_subscriptions.push_back(std::move(subscription));
        }
        m_asts_subscriptions       = std::move(asts_subscriptions      );
        m_asts_subscriptions_index = std::move(asts_subscriptions_index);
    }

    ...
}

Я добавляю if после строчки:

            asts_subscriptions.push_back(std::move(subscription));

            if(table_node.hasAttribute("extra-load-complete-at") && !subscription.m_updates)
            {
                fill_second_snapshots_subscriptions( subscription, table_node.attribute<std::string>( "extra-load-complete-at" ) );
            }
 }
 m_asts_subscriptions       = std::move(asts_subscriptions      );
 m_asts_subscriptions_index = std::move(asts_subscriptions_index);

Сколько-то в run-time пытаюсь понять, почему не получается. Оказалось, был move, который я не заметил.
Вопрос — зачем?
Это обычный конфиг. Здесь не нужно ничего такого экстра-пупер.
Пиши себе обычный понятный код.

Далее, я хотел if без скобок написать. Но нет. По ревью оказалось нельзя.
Вопрос — зачем? Кто-то когда-то написал что скобки нужны. Но тот же clion сразу делает отступ, чтобы случайно не было путаницы.
А получаем нагрузку в паре лишних символов — {}.
Когда смотрим на файл с одной функцией — может и ок. Но если что-то увесистей, то доп символами нагружаем зрительный канал, мозг. Лишним.
Т.е. мы можем получить код, который:
1) Быстро читается
2) Понятный
Но вместо этого пользуемся другой мотивацией.

Сейчас вообще скажу страшное. Архитектор в cpp делал:

using namespace std;

И можно было писать просто:

string str =...
ifstream ifstr =...
whatever

без "std::" каждый раз, Карл!

Я был возмущен! Ведь из каждого угла всех квадратных строений всех известных обитаемых галактик говорится:
"Это bad practice! Never, never do it."

А потом я офигел от того, что из-за отсутствия 3ех букв и двух двоеточий код распрямился, дышал полной грудью и я с ним становился на ты.

А ешё, оказывается, в hpp можно не писать название аргумента, если понятно из типа о чём идет речь:

void fill_second_snapshots_subscriptions(subscription_t &, ...);

Кульминация
Делаю далее.
Стараюсь не запутаться с преобразованием времени.
Пишу многословно, чтобы было понятней прочитать на следующей неделе.
Стараюсь сделать auto(сейчас понял, что сделал не везде):

void Config::fill_second_snapshots_subscriptions(subscription_t & subscription, const std::string & time_from_config)
{
    subscription.m_snapshot = true;

    std::regex input_mask("^[0-2][0-9]:[0-5][0-9]:[0-5][0-9]$");

    if(! regex_match(time_from_config, input_mask))
    {
        std::ostringstream oss_err;
        oss_err << "Invalid time format for a second snapshot "
                << time_from_config
                << " for table "
                << subscription.m_table
                << ". Valid is HH:MM:SS";
        throw std::logic_error(oss_err.str());
    }

    std::time_t raw_time = std::time(nullptr);
    std::tm     current_time_stamp = *std::localtime(&raw_time);

    std::istringstream ss(time_from_config);
    ss >> std::get_time(&current_time_stamp, "%H:%M:%S");

    auto time_stamp_with_time_from_config = std::mktime(&current_time_stamp);

    auto time_to_start_snapshot = system_clock::from_time_t(time_stamp_with_time_from_config);
    auto now = system_clock::now();

    if(time_to_start_snapshot > now )
    {
        subscriptions_second_snapshots[subscription] = time_to_start_snapshot;
    }
}

И получаю на ревью:

const auto& time_stamp_with_time_from_config = std::mktime(&current_time_stamp);
const auto& time_to_start_snapshot = system_clock::from_time_t(time_stamp_with_time_from_config);
const auto& now = system_clock::now();

Т.к. RVO, Карл!
Why?
Why здесь?

Развязка
"Чтобы не терять навык."

Эпилог
Вспоминаю выступление Скотта Мейерса.
Он говорил(на память), что "Есть фича. И есть варианты/контекст её использования". Есть граничные условия.
Что хорошо бы использовать фичу, где её можно использовать надежно, где она к месту.
Это я, наверное, про начальный move, который ни к земному селу, ни к марсианскому городу. Который просто усложнил чтение.

https://youtu.be/9vyh1APsMAE?t=2258

Ок. Есть RVO. Но в конфиге зачем его юзать?

Здравствуйте, avovana, Вы писали:

A>Сколько-то в run-time пытаюсь понять, почему не получается. Оказалось, был move, который я не заметил.
A>Вопрос — зачем?
A>Это обычный конфиг. Здесь не нужно ничего такого экстра-пупер.
A>Пиши себе обычный понятный код.

А чем emplace_back не угодил? По идее и проще и короче и надежнее

    if (!asts_subscriptions_index.emplace(table_node.attribute<std::string>("micex-name"), asts_subscriptions.size()).second)
        continue; // TODO: duplicated table warning.

    asts_subscriptions.emplace_back(
        table_node.attribute<std::string>("micex-name"),
        table_node.attribute<bool>("load-complete" , "true"),
        table_node.attribute<bool>("enable-refresh", "false"),
        table_node.attribute<bool>("enable-publish", "true"));

    if (table_node.hasAttribute("extra-load-complete-at") && !subscription.m_updates)
             fill_second_snapshots_subscriptions(table_node.back(), table_node.attribute<std::string>("extra-load-complete-at"));

Если чтение атрибута дорогая операция, то можно создать временную переменную для "micex-name".

Здравствуйте, avovana, Вы писали:

A>Эпилог
A>Вспоминаю выступление Скотта Мейерса.
A>Он говорил(на память), что "Есть фича. И есть варианты/контекст её использования". Есть граничные условия.
A>Что хорошо бы использовать фичу, где её можно использовать надежно, где она к месту.
A>Это я, наверное, про начальный move, который ни к земному селу, ни к марсианскому городу. Который просто усложнил чтение.

Моя позиция: всем использовать единый стиль оформления кода. Он плох или тебе не нравится? Надо договориться об изменениях. Одному и втихаря даже разумные идеи продвигать вредно для команды и проекта.
Тебе не нравится move для большой std::string? Почему? Есть негласное правило: если потенциально в этом месте не работает short (small) string optimization, то надо использовать move. В твоём случае строка явно большая. Не так важно, что в данном конкретном случае это погоды не сделает, сколько важно, что так делается во всём проекте.
Аналогичных гласных или негласных правил много: писать override, final, default для конструкторов/деструкторов или noexcept, copy ellision или передача по ссылке, инициализация членов класса при объявлении и т.д. Какие-то правила приходят, какие-то уходят, но они должны быть общеприняты и оговорены.

Здравствуйте, avovana, Вы писали:

я предпочитаю не использоовать явно std::move. Лучше заставить компилятор вызвать его неявно.

constexpr auto getSubscription = [](auto const& table_node) { // лучше конечно подставить явный тип table_node

    subscription_t subscription; // Таблица_имя, режим вычитки, ...
    subscription.m_table = table_node.attribute<std::string>("micex-name");
    subscription.m_snapshot = table_node.attribute<bool>("load-complete" , "true" );
    subscription.m_updates  = table_node.attribute<bool>("enable-refresh", "false");
    subscription.m_publish  = table_node.attribute<bool>("enable-publish", "true" );
    return subscription;
};

...

asts_subscriptions.push_back(getSubscription(table_node));

Здравствуйте, avovana, Вы писали:

A>Далее, я хотел if без скобок написать. Но нет. По ревью оказалось нельзя.

В век clang-format и CI кто-то еще ревьювит скобочки

Здравствуйте, koenjihyakkei, Вы писали:

K>Здравствуйте, avovana, Вы писали:

A>>Далее, я хотел if без скобок написать. Но нет. По ревью оказалось нельзя.

K>В век clang-format и CI кто-то еще ревьювит скобочки

Ну, да. Согласен) К этому скоро придём на проекте)

Здравствуйте, Skorodum, Вы писали:

S>А чем emplace_back не угодил? По идее и проще и короче и надежнее

Как вариант. Это было написано до меня. По мне, твой вариант тоже нормально выглядит.

Здравствуйте, Nuzhny, Вы писали:
N>Моя позиция: всем использовать единый стиль оформления кода. Он плох или тебе не нравится? Надо договориться об изменениях. Одному и втихаря даже разумные идеи продвигать вредно для команды и проекта.
Здесь согласен.
Вопрос, скорее, в целом, про варианты использования языка в разных контекстах.

N>Тебе не нравится move для большой std::string? Почему? Есть негласное правило: если потенциально в этом месте не работает short (small) string optimization, то надо использовать move. В твоём случае строка явно большая. Не так важно, что в данном конкретном случае это погоды не сделает, сколько важно, что так делается во всём проекте.
Но вот не было move и как-то жили. По-прежнему, подсвечу основную мысль — это конфиг. Он считывает файл при старте. Ему не нужно это делать за 1,23 ns.
Зачем синтаксически усложнять код?
Учитывая этот контекст использования — "медленный" конфиг, который просто делает своё дело и который хочется написать понятно и чтобы читалось понятно.
Просто сделал сущность. Просто наполнил. Просто положил в контейнер.

N> Аналогичных гласных или негласных правил много: писать override, final, default для конструкторов/деструкторов или noexcept, copy ellision или передача по ссылке, инициализация членов класса при объявлении и т.д. Какие-то правила приходят, какие-то уходят, но они должны быть общеприняты и оговорены.
А, допустим, есть модуль, где нужны плюсы во всей красе — там тебе и copy ellision и т.п.
Так почему из-за пары таких мест, вносить обязательство везде сражаться за RVO

const auto& time_stamp_with_time_from_config = std::mktime(&current_time_stamp);
const auto& time_to_start_snapshot = system_clock::from_time_t(time_stamp_with_time_from_config);
const auto& now = system_clock::now();

порождая во всех модулях вырви-глаз код? Т.к. в общем гайде код стайла так написано(или запечаталось в голове)?

Здравствуйте, avovana

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

A>Но вот не было move и как-то жили.

Не очень хорошо жили, прямо скажем. Хотя кто-то и сейчас готов жрать ложками, но это уже другая история.

A>По-прежнему, подсвечу основную мысль — это конфиг.

Основная мысль -- это гигиена кода. Это как RAII вместо ручного освобождения ресурсов, должно выполняться на автомате.
У вас уже есть созданная строка, зачем её копировать?
Затем, что вы настолько неквалифицированы, что не умеете использовать move и прикрываете свое неумение разговорами про 1.23ns?

Ну, OK. Есть много способов испортить себе репутацию. Вы предлагаете еще один.

A>

A>const auto& time_stamp_with_time_from_config = std::mktime(&current_time_stamp);
A>const auto& time_to_start_snapshot = system_clock::from_time_t(time_stamp_with_time_from_config);
A>const auto& now = system_clock::now();
A>

A>порождая во всех модулях вырви-глаз код? Т.к. в общем гайде код стайла так написано(или запечаталось в голове)?

И ваш исходный, и этот "модифицированный" фрагменты кода вызывают вопросы сами по себе.
Ну вот зачем эта портянка с тремя переменными (а это именно что переменные):

    auto time_stamp_with_time_from_config = std::mktime(&current_time_stamp);

    auto time_to_start_snapshot = system_clock::from_time_t(time_stamp_with_time_from_config);
    auto now = system_clock::now();

    if(time_to_start_snapshot > now )
    {
        subscriptions_second_snapshots[subscription] = time_to_start_snapshot;
    }

когда здесь напрашивается что-то вроде:

    const auto time_to_start_snapshot = system_clock::from_time_t(std::mktime(&current_time_stamp));
    if(time_to_start_snapshot > system_clock::now() )
    {
        subscriptions_second_snapshots[subscription] = time_to_start_snapshot;
    }

И если кому-то захочется переделать объявление time_to_start_snapshot вот таким образом:

const auto & time_to_start_snapshot = ...;

То можно просто вежливо попросить пояснить, чем это будет эффективнее хранения экземпляра маленького POD-типа.

Да и исходный фрагмент парсинга конфига производит впечатление сделанного "на отвали":

void Config::load(xml_wrap::NodeConst const & root_node)
{
    // Call base class first.
    ipc::SimpleConfig::load(root_node("ipc"));

    auto const micex_node = root_node("micex");
    { // (1)
        auto const connection_node = micex_node("connection");
        m_asts_connection_attempts       = connection_node.attribute<unsigned int>("attempts", "6");
        m_asts_connection_attempt_period = std::chrono::seconds(connection_node.attribute<unsigned int>("attempt-period", "300"));

        std::string asts_connection_params;
        auto const param_nodes = connection_node.children("param");
        for(auto const & param_node : param_nodes)
        {
            asts_connection_params += param_node.attribute<std::string>("name");
            asts_connection_params += '=';
            asts_connection_params += param_node.attribute<std::string>("value");
            asts_connection_params += "\r\n";
        }
        m_asts_connection_params = std::move(asts_connection_params);
    }

Фактически, весь блок помеченный меткой (1) выглядит как кандидат в вынос в отдельную подпрограмму. Типа:

void Config::extract_connection_params(xml_wrap::NodeConst const & connection_node)
{
    m_asts_connection_attempts       = connection_node.attribute<unsigned int>("attempts", "6");
    m_asts_connection_attempt_period = std::chrono::seconds(connection_node.attribute<unsigned int>("attempt-period", "300"));

    std::string asts_connection_params;
    auto const param_nodes = connection_node.children("param");
    for(auto const & param_node : param_nodes)
    {
        asts_connection_params += param_node.attribute<std::string>("name");
        asts_connection_params += '=';
        asts_connection_params += param_node.attribute<std::string>("value");
        asts_connection_params += "\r\n";
    }
    m_asts_connection_params = std::move(asts_connection_params);
}
...
void Config::load(xml_wrap::NodeConst const & root_node)
{
    // Call base class first.
    ipc::SimpleConfig::load(root_node("ipc"));

    auto const micex_node = root_node("micex");

    extract_connection_params(micex_node("connection"));
    ...
}

Да и в отдельном уже extract_connection_params запросто можно избавиться от столь неприглядного для вас move:

void Config::extract_connection_params(xml_wrap::NodeConst const & connection_node)
{
    const auto make_asts_connection_params = [&]()
    {
        std::string result;
        auto const param_nodes = connection_node.children("param");
        for(auto const & param_node : param_nodes)
        {
            result += param_node.attribute<std::string>("name");
            result += '=';
            result += param_node.attribute<std::string>("value");
            result += "\r\n";
        }
        return result;
    };

    m_asts_connection_attempts       = connection_node.attribute<unsigned int>("attempts", "6");
    m_asts_connection_attempt_period = std::chrono::seconds(connection_node.attribute<unsigned int>("attempt-period", "300"));
    m_asts_connection_params         = make_asts_connection_params();
}

Собственно, тоже самое можно адресовать и другим блокам кода внутри вашего оригинального Config::load.

Здравствуйте, avovana, Вы писали:

A>Так почему из-за пары таких мест, вносить обязательство везде сражаться за RVO
A>

A>const auto& time_stamp_with_time_from_config = std::mktime(&current_time_stamp);
A>const auto& time_to_start_snapshot = system_clock::from_time_t(time_stamp_with_time_from_config);
A>const auto& now = system_clock::now();
A>

А поясни, в чём конкретно были претензия к исходному и этому коду?
Как раз обязательный RVO в последних стандартах сделал так, что разницы между следующими конструкциями не будет:

T foo(); 

const T& a = foo(); // 1
const T  b = foo(); // 2

То есть как раз писать now = system_clock::now() можно как через значения, так и через константные ссылки — разницы теперь не будет. То есть, вероятно, претензия была к чему-то другому, так как с точки зрения эффективности тут теперь разницы нет, как и нет "сражения за RVO".

Здравствуйте, avovana, Вы писали:

A>Далее, я хотел if без скобок написать. Но нет. По ревью оказалось нельзя.
Довольно распространенная практика.

A>Вопрос — зачем? Кто-то когда-то написал что скобки нужны. Но тот же clion сразу делает отступ, чтобы случайно не было путаницы.
А если он не используется? А если при мерже вставится что-то лишнее?

A>А получаем нагрузку в паре лишних символов — {}.
В контексте с++ лишняя пара {} — это смешно. Тебя же не парит писать 'std::', а это аж 5 символов.

Здравствуйте, AndrewJD, Вы писали:

A>>...тот же clion сразу делает отступ, чтобы случайно не было путаницы.
AJD>А если он не используется? А если при мерже вставится что-то лишнее?
Представим, что используется у всех и здорово помогает программистам в т.ч. такими авт. выравниваниями.
И представим, что есть код ревью, что если уж:
1) Программист там профакапит
2) Clion сломается в тот момент
То ревьювер своей 3ей проверкой вполне себе найдет не правильное выравнивание.

A>>А получаем нагрузку в паре лишних символов — {}.
AJD>В контексте с++ лишняя пара {} — это смешно. Тебя же не парит писать 'std::', а это аж 5 символов.
Ну вот если парит. Как описывал прошлый опыт, когда этих вырвиглаз 5 символов не было, то сначала недоумевал как так.
А потом радовался, потому что можно было быстрее читать код, который стал понятней.
Используешь строку?
Пиши

string str = ("Hello people! It is so easy to read code without distracted symbols std:: and so on");

Используешь вн. переменную, пиши

int cool_variable;

а не вырвиглаз

int m_cool_variable;

Я к тому, что код становится читаемый.
Предлагаю посмотреть именно с этой стороны.

Здравствуйте, avovana, Вы писали:

A>Ну вот если парит. Как описывал прошлый опыт, когда этих вырвиглаз 5 символов не было, то сначала недоумевал как так.
A>А потом радовался, потому что можно было быстрее читать код, который стал понятней.

А я наоборот — везде пишу. Если используется много внешних библиотек, то всё сливается и непонятно что и откуда берётся.
Что использую — сокращения, например вместо std::filesystem использую fs.
У меня в коде много разных namespace: std, boost, cv, nvinfer... Соответственно, я хочу видеть откуда и что используется. Нет ничего — текущий namespace.

A>а не вырвиглаз
A>int m_cool_variable;

Все члены класса с приставкой "m_"!
Посмотришь иной код:

void A::Func(int width, int height)
{
    this->width = width;
    this->height = height;
}

И вдогонку ещё предупреждения от компилятора. Вместо этого:

void A::Func(int width, int height)
{
    m_width = width;
    m_height = height;
}

И всё понятно! В принципе понятно, где локальные переменные, а где члены класса.
Короче, я за понятность, а не краткость.

Здравствуйте, so5team, Вы писали:

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

Здравствуйте, watchmaker, Вы писали:
W>const T& a = foo(); // 1
W>const T b = foo(); // 2
W>[/ccode]

W>То есть как раз писать now = system_clock::now() можно как через значения, так и через константные ссылки — разницы теперь не будет. То есть, вероятно, претензия была к чему-то другому, так как с точки зрения эффективности тут теперь разницы нет, как и нет "сражения за RVO".

Спасибо за комментарий.

W>А поясни, в чём конкретно были претензия к исходному и этому коду?

То есть, изначально попробовался вариант с auto:

auto time_stamp_with_time_from_config = std::mktime(&current_time_stamp);
auto time_to_start_snapshot = system_clock::from_time_t(time_stamp_with_time_from_config);
auto now = system_clock::now();

И фидбек такой, что чтобы было RVO нужно везде "const auto&".

Я же сейчас пытаюсь донести такую мысль.
С++ — прекрасен. На нём можно писать код, который быстро выполняется. Что хорошо бы, как говорит, so5team знать, что
S>Основная мысль -- это гигиена кода. Это как RAII вместо ручного освобождения ресурсов, должно выполняться на автомате.
S>У вас уже есть созданная строка, зачем её копировать?

Предлагаю рассмотреть код — как то, что читают. Совсем его питоновским, конечно, сделать не получится и не надо.
Опять же, вернусь к алгоритму:
1) создали сущность
2) наполнили
3) push_back в контейнер.

so5team напирает на то что будет лишняя копия и это не признак профессионала.
Я же предлагаю расммотреть, что:
1) будет легко читаться
2) в конфиге не страшно делать лишнии копии
3) использование новомодных штук типа move в каждом месте усложняет чтение и усложняет вхождение нового человека — не проффи в плюсах.

По-моему, Лаптев, предлагал отнести move к фичи для продвинутых.
Т.е. хочу, к примеру, команду, джунов, мидлов. Они, как и все мы, как и Страуструп не знают весь С++.
Мы можем договориться писать нормально читаемый код. Что такие вещи, которые оптимизируют, да, но не являются необходимостью здесь, не нужно использовать.

Здравствуйте, Nuzhny, Вы писали:

N>Здравствуйте, avovana, Вы писали:

A>>Ну вот если парит. Как описывал прошлый опыт, когда этих вырвиглаз 5 символов не было, то сначала недоумевал как так.
A>>А потом радовался, потому что можно было быстрее читать код, который стал понятней.

N>А я наоборот — везде пишу. Если используется много внешних библиотек, то всё сливается и непонятно что и откуда берётся.
N>Что использую — сокращения, например вместо std::filesystem использую fs.
N>У меня в коде много разных namespace: std, boost, cv, nvinfer... Соответственно, я хочу видеть откуда и что используется. Нет ничего — текущий namespace.

Да-да. Я согласен, что есть рекомендация. И она хорошая.
Предположим, что на проекте нет одновременно строки от стандартной библиотеки и еще какой-то.
Что нет пересекающихся множеств. Что мало подключаемых библиотек.
Должны тогда, всё-таки, придерживаться этой популярной рекомендации? Которая вполне reasonable в вашем случае.

Если необходимость в её использование отпадает, то аргументы в пользу читаемости кода теперь ничто не перевешивает.

>Посмотришь иной код:
>void A::Func(int width, int height)
>{
> this->width = width;
> this->height = height;
>}

С this больше java стайл, согласен)
Есть такой лайфхак:

void A::Func(int width_, int height_)
{
    width = width_;
    height = height_;
}

Т.е. переменные width, height мы будет использоваться постоянно внутри класса.
И они не вырвиглазные таким образом. А то что добавили постфикс "_" в аргументы — им-то что — один раз для инициализации использовались и забыли.

Здравствуйте, avovana, Вы писали:

A>Насчет отдельной функции есть вопрос. Что если читаем текущую функцию чтения конфига, то она везде здесь перед глазами.

Нет. Перед глазами можно держать 10-15 строк, причем в случае, когда все происходящее в этих строках находится на +- одинаковом концептуальном уровне (грубо говоря, на одном уровне вложенности или около того).

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

A>Добавление новой — в неё теперь нужно входить, узнавать детали. Наверное, на уровне обсуждения, считать ли это деталями или программист, который залез в эту функцию должен и это видеть сразу.

Как раз все наоборот. Вы читаете функцию для того, чтобы понять в общем, что в ней происходит. Это хорошо работает, когда у вас все действия на +- одном уровне вложенности. И когда этих действий немного (отсюда и те самые 10-15 строк).

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

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

A>so5team напирает на то что будет лишняя копия и это не признак профессионала.
A>Я же предлагаю расммотреть, что:
A>1) будет легко читаться
A>2) в конфиге не страшно делать лишнии копии
A>3) использование новомодных штук типа move в каждом месте усложняет чтение и усложняет вхождение нового человека — не проффи в плюсах.

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

Ну а сами плюсы в последние лет 10 стремительно превращаются в язык, который предназначен для проффи и только для тех областей/задач, в которых более простые и удобные инструменты не подходят. Так что аргумент о "простоте восприятия" для C++ уже не работает.

Здравствуйте, avovana, Вы писали:

A>Если необходимость в её использование отпадает, то аргументы в пользу читаемости кода теперь ничто не перевешивает.

Всё равно пишу на автомате.

A>Т.е. переменные width, height мы будет использоваться постоянно внутри класса.
A>И они не вырвиглазные таким образом. А то что добавили постфикс "_" в аргументы — им-то что — один раз для инициализации использовались и забыли.

А как тогда отличить локальные переменные от членов класса?

Здравствуйте, avovana, Вы писали:

AJD>>А если он не используется? А если при мерже вставится что-то лишнее?
A>То ревьювер своей 3ей проверкой вполне себе найдет не правильное выравнивание.
Скорее всего эти правила появились когда небыло clion, когда все еще не сидели на гите. Тот же CVS мог легко смержить так, что было бы несколько строк:

if (condition)
   code_from_brach_a;    
   code_from_brach_b;

и проревьюить мерж на десятки тысяч строк кода еще то удовольствие и такую ошибку легко пропустить.

Сейчас, по мере развития тулзов — это уже не так актуально. Но есть уже код-стандарт и куча легаси кода, где "безобразно", но единообразно.

A>Используешь вн. переменную, пиши
A>

A>int cool_variable;
A>

A>а не вырвиглаз
A>

A>int m_cool_variable;
A>

мне одинаково

. Еще может и такой вариант:

int cool_variable_;

A>Я к тому, что код становится читаемый.
ИМХО, это чисто вкусовщина как пробел vs tab или должна ли скобка открываться на новой строке. Поэтому лучше использовать подход, который устоялся в данной канторе, а не идти войной.

	От:	avovana
	Дата:	09.02.21 09:13
	Оценка:	1 (1)

	От:	kov_serg
	Дата:	09.02.21 11:13
	Оценка:

	От:	Skorodum
	Дата:	09.02.21 12:31
	Оценка:

От:	Nuzhny	https://github.com/Nuzhny007
Дата:	09.02.21 12:46
Оценка:	6 (1) +6

	От:	sergii.p
	Дата:	09.02.21 14:16
	Оценка:

От:	so5team	https://stiffstream.com
Дата:	10.02.21 13:39
Оценка:

	От:	AndrewJD
	Дата:	11.02.21 19:29
	Оценка:

	От:	B0FEE664
	Дата:	11.02.21 21:14
	Оценка: