Хочется feedback на мой код

Question

Rus6lanNov. 1, 2017, 7:27 a.m.

Хочется feedback на мой код

Здравствуйте. Я написал небольшую программу. Хочется, чтобы умные люди посмотрели и сказали, что хорошо, а что плохо. Я был бы очень признателен за такую помощь. Вот гитхаб с репозиторием.

We recommend hosting TIMEWEB

Stable hosting, on which the social network EVILEG is located. For projects on Django we recommend VDS hosting.

Subscribe to the discussion3

Section subscription6

Do you like it? Share on social networks!

Nov. 1, 2017, 9:16 a.m.

Пару маленьких замечаний хоть и не очень принципиальных но все таки:
1. Коментарии желательно писать на английском
2. Дефолтное значение в тегах типа tr("Дизель адмирал") всегда английское.

R

Rus6lan → BlinCT
#

Nov. 1, 2017, 9:29 a.m.

Спасибо большое за фидбэк!

1. Так попросили(

2. Вы имеете ввиду, что при заключении в tr, можно будет использовать локализацию?

Evgenii Legotckoi → Rus6lan
#

Nov. 1, 2017, 9:34 a.m.

2. Вообще, tr используется как раз для переводов приложения, при смене языка приложения там будет подставлен нужный текст.
Вот здесь есть информация о динамическом переводе приложения .

Я ещё не смотрел ваш проект, но гляну его на досуге.

Nov. 1, 2017, 5:06 p.m.
The answer was marked as a solution.

Итак, начнём по порядку.

1. Что бросается в глаза, так это наличие в гит репозитории файлов с расширением *.pro.user.***. Это настроечные файлы проекта. Появляются при первом открытии проектного файла .pro в Qt Creator. В репозитории - это мусор. Их не должно быть здесь. Поэтому настройте gitignore а сами файлы удалите из репозитория.

2. К сожалению, не смог собрать проект, конфликты имен typedef получил в libusb.h, полагаю, что проект делался под Win. И там компилятор проглотил это. GCC не пропустил. Здесь больше ситуативный момент. Поэтому отмечу лишь то, что увижу в коде.

3. mainwindow.cpp
Вот это всё можно настроить через графический дизайнер, чтобы эти строчки не засоряли код. Если используете графический дизайнер, то используйте его по максимуму.

this->setWindowTitle("Дизель-Адмирал");
ui->groupBox->setStyleSheet("QGroupBox { color: blue; } ");
ui->groupBox_2->setStyleSheet("QGroupBox { color: green; } ");
ui->groupBox_3->setStyleSheet("QGroupBox { color: red; } ");

4. mainwindow.cpp

А вот здесь у вас утечка памяти. Создаёте объекты в куче, которые являются наследниками QWidget, а потом не удаляете их в деструкторе или не передаёте при их создании указатель на парента.

Вместо этого

testWidget = new WidgetTest();
settingsWidget = new WidgetSettings();
dieselTypeWidget = new WidgetDieselType();
VMTSetupWidget = new WidgetVMTSetup();
meteringWidget = new WidgetMetering();
selectDieselWidget = new WidgetSelectDiesel();
archiveWidget = new WidgetArchive();

Записать так

testWidget = new WidgetTest(this);
settingsWidget = new WidgetSettings(this);
dieselTypeWidget = new WidgetDieselType(this);
VMTSetupWidget = new WidgetVMTSetup(this);
meteringWidget = new WidgetMetering(this);
selectDieselWidget = new WidgetSelectDiesel(this);
archiveWidget = new WidgetArchive(this);

5. mainwindow.cpp

Название классов виджетов. Лучше записать полностью в английском варианте, то есть

вместо WidgetArchive записать так ArchiveWidget

6. widgetmetering.cpp

Не используйте foreach, это устаревший Qt-шный макрос. В 11-м стандарте C++ есть для этого цикл for.

Вместо

foreach (auto item, radioButtons) { }

Запишите так

for (auto item : radioButtons) { }

7. widgetdieseltype.cpp

применяйте постфиксный инкремент только тогда, когда он действительно нужен

Вместо

for(int i = 0; i < count; i++){ }

Запишите так

for(int i = 0; i < count; ++i){ }

8. Code Style во всех файлах

Пробел между if for while и скобками.

Вместо

if()
for()
while()

То есть писать так

if ()
for ()
while ()

Тоже самое относится к оператору else и фигурным скобками. Также есть такое и при операциях сложения и т.д. Тяжело читать такой сплошной текст.

9. settings.cpp

Думаю, что вместо такого

return QString::fromUtf8(("Channel_" + std::to_string(settings.value("dup").toInt())).c_str());

можно так сделать

return QString("Channel_%1").arg(settings.value("dub").toInt());

10. Используйте новый синтаксис сигналов и слотов на указателях, вместо старого на макросах.

Не надо этого

connect(ui->plot->xAxis, SIGNAL(rangeChanged(QCPRange)), ui->plot->xAxis2, SLOT(setRange(QCPRange)));

Ну пока Вам этого хватит, исправляйте везде ;-)

Там ещё много, это то, что первое в глаза бросилось.

R

Rus6lan → Evgenii Legotckoi
#

Nov. 2, 2017, 4:01 a.m.

Спасибо огромное! Буду ждать еще)

Evgenii Legotckoi → Rus6lan
#

Nov. 2, 2017, 4:04 a.m.

Ну Вы поправьте то, что я указал, залейте новую версию и дайте знать, тогда посмотрю ещё раз.

R

Rus6lan → Evgenii Legotckoi
#

Nov. 2, 2017, 4:46 a.m.

Почти все исправил, но когда дошел до:

testWidget = new WidgetTest(this);
settingsWidget = new WidgetSettings(this);
dieselTypeWidget = new WidgetDieselType(this);
VMTSetupWidget = new WidgetVMTSetup(this);
meteringWidget = new WidgetMetering(this);
selectDieselWidget = new WidgetSelectDiesel(this);
archiveWidget = new WidgetArchive(this);

Вспомнил почему не стал передавать указатель родителя. Потому что получается вот так как на скрине. Он видимо пытается разместить все виджеты на mainwindow.

А когда я ухожу от макросов в сторону указателей:

connect(ui->plot->xAxis, &QCPAxis::rangeChanged(QCPRange), ui->plot->xAxis2, &QCPAxis::setRange(QCPRange));

Он ругается:

error: expected primary-expression before '*' token
     connect(ui->plot->xAxis, &QCPAxis::rangeChanged(QCPRange*), ui->plot->xAxis2, &QCPAxis::setRange(QCPRange*));
                                                             ^

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

Кстати такая запись тоже вызывает ошибку:

connect(ui->plot->xAxis, &QCPAxis::rangeChanged, ui->plot->xAxis2, &QCPAxis::setRange);

Но другую:

no matching function for call to 'WidgetVMTSetup::connect(QCPAxis*&, <unresolved overloaded function type>, QCPAxis*&, <unresolved overloaded function type>)'
     connect(ui->plot->xAxis, &QCPAxis::rangeChanged, ui->plot->xAxis2, &QCPAxis::setRange);
                                                                                          ^

Evgenii Legotckoi → Rus6lan
#

Nov. 2, 2017, 4:45 p.m.

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

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

Что касается сигналов и слотов, то там проблема в том, что они перегруженные. Просто нужно кастовать их :-)

Вот пример для QComboBox, у которого есть перегруденные сигналы также

connect(comboBox, static_cast<void(QComboBox::*)(int)>(&QComboBox::activated),
    [=](int index){ /* ... */ });

Выглядит конечно, страшновато, но лучше всё-таки использовать новый синтаксис. Чем меньше макросов в коде, тем лучше.

R

Rus6lan → Evgenii Legotckoi
#

Nov. 3, 2017, 5:16 a.m.

Я все через дизайнер делаю. В mainwindow находятся кнопки по нажатию на которые должен открывается соответствующий виджет просто в новом окне(да я знаю про StackedWidget и TabWidget, просто так надо). Как сделать так, чтобы виджеты были привязаны к родителю (mainwindow), но не размещались в нем.

R

Rus6lan → Evgenii Legotckoi
#

Nov. 3, 2017, 5:17 a.m.

Вот при нажатии "Выбор".

Evgenii Legotckoi → Rus6lan
#

Nov. 3, 2017, 5:40 a.m.

Ах. Вон оно что. Так Вам тогда нужно наследоваться от классов QDialog. Так будет правильнее. Диалог не должен размещаться внутри окна как обычные виджеты. А открывать его можете либо методам open() , либо exec() , либо show().

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

Ну и плюс, рассматривайте вариант выделения памяти в стеке, а не в куче через оператор new .

R

Rus6lan → Evgenii Legotckoi
#

Nov. 3, 2017, 6:24 a.m.

Ну в стеке же не хорошо, он же небольшой. В деструкторе просто написать:

MainWindow::~MainWindow()
{
    delete WidgetArchive;
    delete WidgetDieselType;
    delete WidgetMetering;
    delete WidgetSelectDiesel;
    delete WidgetSettings;
    delete WidgetTest;
    delete WidgetVMTSetup;
    delete ui;
}

А может можно просто создавать не обычные указатели, а например std::shared_ptr?

Evgenii Legotckoi → Rus6lan
#

Nov. 3, 2017, 6:29 a.m.
(edited)

Тогда в деструкторе.
std::shared_ptr - на ваше усмотрение.

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

R

Rus6lan → Evgenii Legotckoi
#

Nov. 3, 2017, 6:38 a.m.

Спасибо большое!

R

Rus6lan → Evgenii Legotckoi
#

Nov. 4, 2017, 10:06 a.m.

Я вроде много чего исправил. Только макросы не переделал. Есть еще какие-то серьёзные ошибки?

Evgenii Legotckoi → Rus6lan
#

Nov. 4, 2017, 10:22 a.m.

Позже посмотрю, как время будет. Сегодня работаю.

Nov. 6, 2017, 2:52 a.m.
(edited)

Посмотрел проект ещё раз.

1. Всё-таки какое-то кривое у Вас подключение библиотеки libusb. Попытался собрать под Windows. Выдало следующую ошибку:

D:\Code Review\Qt_Diesel_Analyze\libusb\include\libusb-1.0\libusb.h:44: error: C2371: 'ssize_t': redefinition; different basic types

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

2. Наличие закоментированного кода. Понимаю, что это издержки разработки проекта, но придерживаюсь такой политики, что весь закоментированный код должен удаляться. Кроме некоторых случаев, когда он понадобится после завершения некоторых User Story или багфиксов.

3. Деструкторы лучше прописывать виртуальными. В некоторых местах они объявлены без ключевого слова virtual.

4. Когда создаёте классы, в Qt Creator, то он создаёт следующее объявление конструктора для классов, наследованных от QObject.

explicit WidgetVMTSetup(QWidget *parent = 0);

Лучше сразу переписывать таким образом:

explicit WidgetVMTSetup(QWidget *parent = nullptr);

В современном стандарте C++ введено ключевое слово nullptr для нулевого указателя. Вот его лучше и использовать в качестве значения по умолчанию. Иначе иногда можно получить неопределённое поведение, зависящее от платформы разработки. В рамках самого Qt вряд ли получите эту проблему, но если будете ещё что-то делать с парентом в конструкторе, то можно получить головной боли.

5. Не знаю, насколько Вам важна возможность локализации проекта, но забыли обернуть в метод tr кое-где текст в проекте.

Было

ui->plot->yAxis->setLabel("Кг/см2");
ui->plot->xAxis->setLabel("Угол поворота К.В.");

Сделать так

ui->plot->yAxis->setLabel(tr("Кг/см2"));
ui->plot->xAxis->setLabel(tr("Угол поворота К.В."));

6. Подумайте над тем, чтобы здесь применить либо статические константные переменные QString для ключей, либо вообще перечисления enum .

ui->plot->graph()->setData(countVector, mapContainer["Channel_1"]);

То есть вместо Channel_1, либое переменную, либо enum. Пока проект небольшой - это не сильно напрягает, а когда становится большим, то с такой записью ключей это будет сплошная морока. Лучше иметь какой-нибудь

static const QString CHANNEL_1 = "Channel_1";

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

7. Конечно, даже стандарт кодстайла Qt позволяет не использовать фигурные скобки в теле условного блока, если там всего одна строка. Но случайно добавленная строчка можно прибавить головной боли. Поэтому лучше всегда оборачивать всё в фигурные скобки, если даже там одна строка.

Было

if (child->layout() != 0) clearLayout(child->layout());
        else if (child->widget() != 0) delete child->widget();

Стало

if (child->layout() != 0) 
{
    clearLayout(child->layout());
}
else if (child->widget() != 0) 
{
    delete child->widget();
}

8. Возвращаясь к предыдущему коду

метод layout() возвращает указатель. В современном стандарте C++ это nullptr , то есть проверку нужно делать на на 0, который платформозависимый, а на nullptr

Было

if (child->layout() != 0)

Стало

if (child->layout() != nullptr)

При этом даже эту запись можно сократить

if (child->layout())

Получается корректный код, который будет работать как надо.

9. Файлы библиотеки ModBusUSB.h имеют проблемы с кодировкой, нужно пересохранить в UTF8, в противном случае у меня там кракозябры.

10. Файл dieseltype.h . Все заголовочные файлы, объекты которых не объявляются в стеке необходимо перенести в файл исходных кодов. Для тех типов, которые имеют только указатель, можно использовать Forward Declaration а заголовочники опять же подключать в cpp файле.

Профит в ускорении компиляции и решении некоторых проблем с линковкой в сложных проектах.

Comments

Only authorized users can post comments.
Please, Log in or Sign up