R
Rus6lan1 ноября 2017 г. 7:27

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

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

Рекомендуем хостинг TIMEWEB
Рекомендуем хостинг TIMEWEB
Стабильный хостинг, на котором располагается социальная сеть EVILEG. Для проектов на Django рекомендуем VDS хостинг.

Вам это нравится? Поделитесь в социальных сетях!

17
BlinCT
  • 1 ноября 2017 г. 9:16

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

    R
    • 1 ноября 2017 г. 9:29

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

    1. Так попросили(
    2. Вы имеете ввиду, что при заключении в tr, можно будет использовать локализацию?
      Evgenii Legotckoi
      • 1 ноября 2017 г. 9:34

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


      Я ещё не смотрел ваш проект, но гляну его на досуге.
        Evgenii Legotckoi
        • 1 ноября 2017 г. 17:06
        • Ответ был помечен как решение.

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


        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
          • 2 ноября 2017 г. 4:01

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

            Evgenii Legotckoi
            • 2 ноября 2017 г. 4:04

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

              R
              • 2 ноября 2017 г. 4:46

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


              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
                • 2 ноября 2017 г. 16:45

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

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

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

                Вот пример для QComboBox, у которого есть перегруденные сигналы также
                connect(comboBox, static_cast<void(QComboBox::*)(int)>(&QComboBox::activated),
                    [=](int index){ /* ... */ });
                Выглядит конечно, страшновато, но лучше всё-таки использовать новый синтаксис. Чем меньше макросов в коде, тем лучше.
                  R
                  • 3 ноября 2017 г. 5:16

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

                    R
                    • 3 ноября 2017 г. 5:17

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

                      Evgenii Legotckoi
                      • 3 ноября 2017 г. 5:40

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

                      Смотрите по обстоятельствам. Но обычзательно удаляйте виджеты в деструкторе окна, если всё-таки оставите свой первый вариант.
                      Ну и плюс, рассматривайте вариант выделения памяти в стеке, а не в куче через оператор new .
                        R
                        • 3 ноября 2017 г. 6:24

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

                        MainWindow::~MainWindow()
                        {
                            delete WidgetArchive;
                            delete WidgetDieselType;
                            delete WidgetMetering;
                            delete WidgetSelectDiesel;
                            delete WidgetSettings;
                            delete WidgetTest;
                            delete WidgetVMTSetup;
                            delete ui;
                        }
                        А может можно просто создавать не обычные указатели, а например std::shared_ptr?
                          Evgenii Legotckoi
                          • 3 ноября 2017 г. 6:29
                          • (ред.)

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

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

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

                              R
                              • 4 ноября 2017 г. 10:06

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

                                Evgenii Legotckoi
                                • 4 ноября 2017 г. 10:22

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

                                  Evgenii Legotckoi
                                  • 6 ноября 2017 г. 2:52
                                  • (ред.)

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


                                  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 файле.
                                  Профит в ускорении компиляции и решении некоторых проблем с линковкой в сложных проектах.

                                    Комментарии

                                    Только авторизованные пользователи могут публиковать комментарии.
                                    Пожалуйста, авторизуйтесь или зарегистрируйтесь
                                    B

                                    C++ - Тест 002. Константы

                                    • Результат:16баллов,
                                    • Очки рейтинга-10
                                    B

                                    C++ - Тест 001. Первая программа и типы данных

                                    • Результат:46баллов,
                                    • Очки рейтинга-6
                                    FL

                                    C++ - Тест 006. Перечисления

                                    • Результат:80баллов,
                                    • Очки рейтинга4
                                    Последние комментарии
                                    k
                                    kmssr9 февраля 2024 г. 2:43
                                    Qt Linux - Урок 001. Автозапуск Qt приложения под Linux как сделать автозапуск для флэтпака, который не даёт создавать файлы в ~/.config - вот это вопрос ))
                                    АК
                                    Анатолий Кононенко5 февраля 2024 г. 9:50
                                    Qt WinAPI - Урок 007. Работаем с ICMP Ping в Qt Без строки #include <QRegularExpressionValidator> в заголовочном файле не работает валидатор.
                                    EVA
                                    EVA25 декабря 2023 г. 18:30
                                    Boost - статическая линковка в CMake проекте под Windows Ошибка LNK1104 часто возникает, когда компоновщик не может найти или открыть файл библиотеки. В вашем случае, это файл libboost_locale-vc142-mt-gd-x64-1_74.lib из библиотеки Boost для C+…
                                    J
                                    JonnyJo25 декабря 2023 г. 16:38
                                    Boost - статическая линковка в CMake проекте под Windows Сделал всё по-как у вас, но выдаёт ошибку [build] LINK : fatal error LNK1104: не удается открыть файл "libboost_locale-vc142-mt-gd-x64-1_74.lib" Хоть убей, не могу понять в чём дел…
                                    G
                                    Gvozdik19 декабря 2023 г. 5:01
                                    Qt/C++ - Урок 056. Подключение библиотеки Boost в Qt для компиляторов MinGW и MSVC Для решения твой проблемы добавь в файл .pro строчку "LIBS += -lws2_32" она решит проблему , лично мне помогло.
                                    Сейчас обсуждают на форуме
                                    P
                                    Pisych27 февраля 2023 г. 12:04
                                    Как получить в массив значения из связанной модели? Спасибо, разобрался:))
                                    AC
                                    Alexandru Codreanu19 января 2024 г. 19:57
                                    QML Обнулить значения SpinBox Доброго времени суток, не могу разобраться с обнулением значение SpinBox находящего в делегате. import QtQuickimport QtQuick.ControlsWindow { width: 640 height: 480 visible: tr…
                                    BlinCT
                                    BlinCT27 декабря 2023 г. 16:57
                                    Растягивать Image на парент по высоте Ну и само собою дял включения scrollbar надо чтобы был Flickable. Так что выходит как то так Flickable{ id: root anchors.fill: parent clip: true property url linkFile p…
                                    Дмитрий
                                    Дмитрий10 января 2024 г. 12:18
                                    Qt Creator загружает всю оперативную память Проблема решена. Удалось разобраться с помощью утилиты strace. Запустил ее: strace ./qtcreator Начал выводиться весь лог работы креатора. В один момент он начал считывать фай…
                                    Evgenii Legotckoi
                                    Evgenii Legotckoi12 декабря 2023 г. 14:48
                                    Побуквенное сравнение двух строк Добрый день. Там случайно не высылается этот сигнал textChanged ещё и при форматировани текста? Если решиать в лоб, то можно просто отключать сигнал/слотовое соединение внутри слота и …

                                    Следите за нами в социальных сетях