[prog.c++.flame] Увидел на Хабре код, от которого меня лично слегка передергивает

Заглянул на Хабре в статью "Оптимизация GUI на Qt" (сам не знаю почему, т.к. на Qt уже очень давно не программировал). Увидел там пример кода... Это как раз образчик кода, от которого меня передергивает после многих десятилетий в программизме и опыта приведения в чувство чужих копролитов.

Сам пример достаточно объемный, я его приведу в конце. А претензии выскажу сразу же, дабы не тянуть кота за хвост. Тем более, что их не так уж и много (по крайней мере при беглом просмотре кода).

---

Итак, первая претензия, она же самая главная: способ обработки элементов из std::variant. Как по мне, так цепочка if-ов в которых вызывается std::get_if для того, чтобы проверить очередной вариант, -- это худшее, что можно сделать.

Почему?

А потому, что такой код плохо переживает изменение состава variant. Вот добавят в variant новый тип и?

И ничего компилятор по этому поводу не скажет. Хорошо, если разработчик вспомнит про места, где он перебирал варианты через std::get_if. А если не вспомнит?

Поэтому гораздо лучше взять std::visit и сделать или функтор с несколькими operator() под каждый из вариантов, или же использовать трюк с overloaded, который описан прямо в cppreference.

В этом случае при изменении состава variant-а компилятор сам будет бить разработчика по рукам и автоматически укажет на все места в программе, которые нужно будет отредактировать.

---

Претензия вторая, тоже серьезная: слишком много кода находится в if-ах в методе setContent. По хорошему, все это нужно выносить в отдельные методы, каждый из которых будет не больше одной странички. Зато все будет намного понятнее и лаконичнее.

---

Следующая претензия мелкая, связанная с моей любовью к const-у. В методе setContent в двух местах объявляется локальная переменная title типа QString. Но она не const, хотя, насколько я могу судить по коду, ее содержимое и не меняется, и не перемещается куда-либо. Т.е. const напрашивается.

В принципе, в setContent есть ряд локальных переменных (особенно внутри if-ов), которые явно служат в качестве констант, но таковыми почему-то не являются.

Почему для меня это так критично?

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

Читаешь код еще раз еще внимательнее и обнаруживаешь, что нет, модификаций нет. Ну и жалко потраченного времени. Поставь автор модификатор const сразу, насколько бы проще бы было.

---

Еще одна мелкая претензия. Это использование int-а для итерации по вектору.

Да, я понимаю, что тема int vs unsigned для индексов и размерностей -- это неоднозначная тема. Серьезные доводы есть и для int, и для unsigned. Сам я убежденный приверженец unsigned, но не в этом суть.

Суть в том, что int в качестве индексов оправдывает себя в некоторых алгоритмах (в большинстве своем математических) в которых допустимо прыгать по индексам туда-обратно. И при прыжке обратно проще получить отрицательное значение и простой проверкой обработать этот вариант, нежели контролировать значение беззнакового индекса перед прыжком обратно.

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

Возможно, эта претензия здесь вообще лишняя, т.к. row затем передается в Qt-шные методы, которые наверняка ждут int, а не unsigned... Но осадочек, как говорится, остался.

Кроме того, при компиляции GCC/Clang на высоком уровне предупреждений наверняка вылезет предупреждение от компилятора по поводу сравнения signed с unsigned вот здесь:

for (int row = 0; row < variants.size(); row++)

А игнорирование предупреждений компилятора не есть хорошо.

---

Следующая претензия также мелкая. Она связана с тем, что вначале setContent объявляется переменная fm типа QFontMetrics. Затем в двух ветках if-а объявляется новая переменная типа QFontMetrics так же с именем fm. При том, что буквально несколькими строчками выше использовалась "старая" fm:

// calculate sizes int pixelsWide = fm.horizontalAdvance(title); maxW1 = std::max<int>(pixelsWide, maxW1); QFontMetrics fm(item->font()); auto rect = fm.boundingRect(QRect(0, 0, 1000, 1000),    Qt::AlignTop | Qt::AlignLeft, content->value);

С точки зрения C++ здесь нет никаких проблем, т.к. fm из вложенного блока скрывает fm из объемлющего блока. Но вот с точки зрения сопровождения кода это не есть хорошо, т.к. стоит поменять в коде какие-то строчки местами и можно нарваться на сюрпризы, т.к. fm есть, но это не тот fm.

Выход здесь простой: дать второму fm какое-то другое название.

Кстати говоря, если мне не изменяет склероз, Clang должен ругаться на подобные вещи.

---

И еще одна мелкая придирка, связанная с явной спецификацией типа шаблона при вызове std::max. Например:

maxW1 = std::max<int>(pixelsWide, maxW1);

Моя логика такова: std::max автоматически выводит T, если типы аргументов одинаковы. Надобность в явной спецификации типа возникает когда аргументы принадлежат разным типам, причем таким, которые друг к другу автоматически не приводятся (например, один unsigned int, а второй short).

Но в таких случаях, как по мне, лучше явным образом привести один из аргументов к типу другого, вроде:

r = std::max(static_cast<unsigned int>(short_arg), unsigned_int_arg);

В таком случае лично мне гораздо понятнее что к чему и почему.

А вот писать std::max<int>... Логика мне не понятна. Тем более, что как я могу судить, там везде именно что int.

---

В общем, резюмируя: нормальный такой средний C++ный код. Который, как мне думается, потенциально опасен геморроем в долгосрочной перспективе. При том, что сделать из него хороший C++ный код совсем не сложно: std::visit вместо if-ов с std::get_if, декомпозиция на более мелкие фрагменты и применение const-а для того, что в принципе не должно меняться.

И ведь, как показывает моя практика, это все реально не сложно.

---

Ну а вот и сам код:

void TwoColumnWgt::setContents(const std::vector<TcwVariant>& variants) {    QFontMetrics fm(this->font());    int      maxW1 = 0;    int      maxW2 = 0;    int      maxSpanW = 0;    int      pixelsHigh = fm.height();    int      hOffset = pixelsHigh / 2;    QString  sepText = separatorText(variants);    setRowCount(variants.size());    for (int row = 0; row < variants.size(); row++)    {       if (const TcwContent* content = std::get_if<TcwContent>(&variants[row]))       {          // add left margin          QString title = " " + content->title;          // add two cells          setItem(row, 0, new QTableWidgetItem(title));          auto* item = new QTableWidgetItem;          item->setData(0, content->value);          setItem(row, 1, item);          // calculate sizes          int pixelsWide = fm.horizontalAdvance(title);          maxW1 = std::max<int>(pixelsWide, maxW1);          QFontMetrics fm(item->font());          auto rect = fm.boundingRect(QRect(0, 0, 1000, 1000),             Qt::AlignTop | Qt::AlignLeft, content->value);          maxW2 = std::max<int>(rect.width(), maxW2);          // correct row height          verticalHeader()->resizeSection(row, rect.height() + hOffset);       }       else if (const TcwHeader* header = std::get_if<TcwHeader>(&variants[row]))       {          // add left margin          QString title = " " + header->title;          // add cell          setSpan(row, 0, 1, 2);          auto* item = new QTableWidgetItem;          auto font = item->font();          font.setBold(true);          constexpr double fontRatio = 10.0 / 8.25;          int pixelSize = font.pixelSize();          double pointSizeF = font.pointSizeF();          if (pointSizeF != -1)             font.setPointSizeF(pointSizeF * fontRatio);          else             font.setPixelSize(qRound(pixelSize * fontRatio));          item->setFont(font);          item->setData(0, title);          setItem(row, 0, item);          // calculate sizes          QFontMetrics fm(font);          int hw = fm.horizontalAdvance(title) + fm.horizontalAdvance("1");          maxSpanW = std::max<int>(hw, maxSpanW);       }       else if (const TcwSeparator* separator = std::get_if<TcwSeparator>(&variants[row]))       {          // add cell          setSpan(row, 0, 1, 2);          auto* item = new QTableWidgetItem;          item->setData(0, sepText);          setItem(row, 0, item);          // correct row height          verticalHeader()->resizeSection(row, hOffset);       }    }    //correct column sizes    maxW1 += fm.horizontalAdvance("12");    horizontalHeader()->resizeSection(0, maxW1);    if (maxW2 > maxSpanW - maxW1)       resizeColumnToContents(1);    else       horizontalHeader()->resizeSection(1, maxSpanW - maxW1); } QString TwoColumnWgt::separatorText(const std::vector<TcwVariant>& variants) const {    // calculate column sizes    int column1Size = 0;    int column2Size = 0;    for (auto& variant : variants)    {       if (auto* content = std::get_if<TcwContent>(&variant))       {          column1Size = std::max<int>(content->title.size(), column1Size);          auto list = content->value.split('\n');          for (auto item : list)             column2Size = std::max<int>(item.size(), column2Size);       }    }    // add left margin    return " " + QString(column1Size + column2Size, '-'); }

Где тип TcwVariant определяется следующим образом:

struct TcwContent {    QString title;    QString value; }; struct TcwHeader {    QString title; }; struct TcwSeparator { }; using TcwVariant = std::variant<TcwContent, TcwHeader, TcwSeparator>;