LINUX.ORG.RU

Junior, Trainee или Никто.

 


0

1

Здравствуйте уважаемый форумчане. Я начинающий с++ программист. Написал небольшую программу. Хочется получить фидбэк по ней, поскольку очень хочется развиваться и понять, что делаю не так, а что так. Буду очень благодарен за фидбэк! Гитахаб - https://github.com/MagistrYodaa/Qt_Diesel_Analyze

Перемещено leave из job

Глянул main.cpp

1. Ты знаешь разницу между

#include <>
и
#include ""
? Гугли.

2.

 QString *datadir = new QString("Data");
Зачем? Ты знаешь разницу между стеком и кучей? Область жизни автоматической переменной?

3.

C:\\Users\\Ruslan\\YandexDisk\\Garant_v2\\release\\icon.png

Всё, вопросов больше нет.

fluorite ★★★★★
()
Ответ на: комментарий от fluorite

1. Погуглил насколько я понял, «» начинает искать файлы в локальных директориях, <> начинает искать файлы в глобальных директориях.

2. Для того, чтобы не висела в стеке. Выделил в куче (сделал директорию) и освободил память. Что плохого?

3. Согласен выглядит не очень, надо переделать.

Спасибо большое за критику!

Rus6lan
() автор топика
Ответ на: комментарий от Rus6lan

Плохо делать ненужное. Как с точки зрения программиста (читать лишнее, потенциальная утечка памяти и т.д.), так и с точки зрения компилятора

struct A {
  int _a = 42;
  A(int a) : _a(a) {}
};

void foo_stack() {
A a { 42 };
}

void foo_heap() {
A *b = new A(42);
delete b;
}

A::A(int):
  push rbp
  mov rbp, rsp
  mov QWORD PTR [rbp-8], rdi
  mov DWORD PTR [rbp-12], esi
  mov rax, QWORD PTR [rbp-8]
  mov edx, DWORD PTR [rbp-12]
  mov DWORD PTR [rax], edx
  nop
  pop rbp
  ret
foo_stack():
  push rbp
  mov rbp, rsp
  sub rsp, 16
  lea rax, [rbp-4]
  mov esi, 42
  mov rdi, rax
  call A::A(int)
  nop
  leave
  ret
foo_heap():
  push rbp
  mov rbp, rsp
  push rbx
  sub rsp, 24
  mov edi, 4
  call operator new(unsigned long)
  mov rbx, rax
  mov esi, 42
  mov rdi, rbx
  call A::A(int)
  mov QWORD PTR [rbp-24], rbx
  mov rax, QWORD PTR [rbp-24]
  mov esi, 4
  mov rdi, rax
  call operator delete(void*, unsigned long)
  nop
  add rsp, 24
  pop rbx
  pop rbp
  ret
fluorite ★★★★★
()
Ответ на: комментарий от fluorite

Спасибо большое! Просто я не очень понимаю, до какого момента будет жить переменная которая объявлена в main? Пока я не закрою программу?

Rus6lan
() автор топика
Ответ на: комментарий от Rus6lan

Внешние зависимости таскать с собой в репозиторий — плохой тон... тем более распространенные типа libusb и в бинарниках.

cetjs2 ★★★★★
()
Ответ на: комментарий от Rus6lan

Пока поток выполнения не выйдет из main, то есть, когда завершится выполнение return.

Ну или когда ты закроешь программу.

В твоём случае, это одно и то же, ибо закрытие программы - это завершение работы функции a.exec() (т.е. выход из области видимости main).

Deleted
()
Последнее исправление: merhalak (всего исправлений: 1)
Ответ на: комментарий от Deleted

Ну вот, это значит что они все это время будут висеть в стеке. Это не хорошо? Лишний раз забивать стек ради такого?

Rus6lan
() автор топика

Я считаю, что за такое надо томиком Кнута по голове бить.

И да, причем тут этот раздел?

fornlr ★★★★★
()
Последнее исправление: fornlr (всего исправлений: 1)
Ответ на: комментарий от Rus6lan

Это будет иметь значение для тех кто читает код. Сразу видно время жизни переменной лежащей в стеке, но когда переменная в куче, то подразумевается что с ней происходит что-то особенное и разбирающий код потратит время на выяснение причин. В данном случае просто подумает WTF?

Txe
()

По-поводу DieselType. 1. У тебя там один сеттеры и геттеры, что вроде бы неплохо, но т.к. они ничего не делают и другого функционала в классе нет, то я просто бы сделал чистую структуру с открытыми членами класса. Просто не имеет смысла городить огород на ровном месте и код выглядит опрятней. 2. Старайся именовать приватный и вообще члены класса отлично от локальных переменных. Так проще читать код когда функция не в одну строчку. Многие используют либо m_Var или m_varName или _Var, в общем тебе решать.

Txe
()

settings.h/cpp просто отличный пример для того что можно улучшить :)

1. Почему это отдельные функции? У нас же объектно ориентированный подход. Если даже хочется просто отдельные функции, то все равно лучше положить их в отдельный namespace (улучшается читабельность). Но в этом конкретном случае, лучше создать класс.

2. Далее идет постоянный повтор кода: создание директории и получение файла конфигурации. Логично выделить это в отдельную функцию и просто ее вызывать и не придется потом проверять не ошибся ли ты где-нибудь в имени директории.

Txe
()
Ответ на: комментарий от cetjs2

Не согласен. Плохой тон - не делать опции сборки с ванильными либами и не указывать их источник. Но и бросать пользователя наедине с битыми ссылками не гоже.

В директорию 3rdparty кинуть бандлы и через 10 лет, когда наткнешься на архив с исходниками - просто запускаешь make и не паришь мозги где взять ту самую версию либы и почему её удалили с гитхаба за противоречие сомнительным законам и проискам копирастов, или просто 404 без затей, как это _всегда_ бывает в интернетах.

Deleted
()
Ответ на: комментарий от Deleted

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

Txe
()

По-поводу DieselType и QVariantMap. Лучше «научить» DieselType сериализоваться в json и выкинуть QVariantMap.

1. Это уменьшит количество сущносностей, QVariantMap у тебя выполняет работу, которую должен делать DieselType.

2. Код становится более типизированный и уберет такие вещи как diesel.insert(«dieseltype», bluh bluh). Нехорошо использовать строки как имена переменных, т.к. легко совершить ошибку, пусть компилятор делает за тебя работу.

Txe
()

опять settings.cpp

У тебя есть в WidgetSettings::on_pushButtonAccept_clicked() есть чтение json и затем

settings.setValue("treshold",ui->spinBoxTreshold->value());
и в тоже время используется
double getTreshold()

Тебе надо создать нормальный класс Settings. Добавить туда setTreshold/getTreshold. Выкинуть из WidgetSettings::on_pushButtonAccept_clicked() работу с json

Txe
()

Есть большой файл dataio.cpp, но давай я выскажу что можно там поправить, когда уже упомянутые недочеты будут приняты :)

Txe
()

Лучше полистай hh и посмотри на какие языки есть спрос. Это C#, Java и JS. Брось ты эти плюсы.

Xunnu ★★
()

main.cpp. Сразу вопрос: почему не используешь QSettings, а велосипедишь вместо этого?

MainWindow.h/.cpp. Ты, часом, на delphi не пишешь? on_buttonMetering2_clicked() — так называть методы нельзя даже в пьяном угаре. Хорошее соглашение: название должно описывать, что произойдёт, и начинаться с глагола. Например, в указанном слоте ты делаешь testWidget->show(), значит он должен называться showTestWidget(). А если подумать чуть дольше, то становится ясно, что этот слот вообще не нужен, так как функция show() сама по себе является слотом, и можно сигналы слать непосредственно ей. Непонятно, почему виджет с настройками (settingsWidget->show();) не сделан в виде диалога, так было бы гораздо логичнее (и тут уже отдельный слот тебе действительно понадобится). Посмотри документацию к QDialog и QDialogButtonBox, чтобы понять, как такие вещи делаются.

Дальше не смотрел

XMs ★★★★★
()
Ответ на: комментарий от XMs

main.cpp. Сразу вопрос: почему не используешь QSettings, а велосипедишь вместо этого?

В этом конкретном случае лучше оставить велосипед, чисто для тренировки на мышах, разные архитектурые решения заодно проработаются :)

Txe
()
Ответ на: комментарий от Txe

Спасибо большое за советы.

1. Почему это отдельные функции? У нас же объектно ориентированный подход. Если даже хочется просто отдельные функции, то все равно лучше положить их в отдельный namespace (улучшается читабельность). Но в этом конкретном случае, лучше создать класс.

Вы имеете ввиду класс с статическими методами?

Rus6lan
() автор топика
Ответ на: комментарий от Txe

Ох уж эти проблемы крестодетей без менеджера зависимостей с централизованным хранилищем и семантическим версионированием.

mersinvald ★★★★★
()
Ответ на: комментарий от Rus6lan

Вы имеете ввиду класс с статическими методами?

Можно и статическими методами. Хотя я думал про обычные, ведь можно сделать QSettings членом класса и инициализировать его один раз, а не на каждый вызов метода. Но общая идея в том, что бы делегирование ответственности было в одном месте (в данном случае работа с ini файлом), а не размазано по всему проекту.

Txe
()
Ответ на: комментарий от Rus6lan

Ну если его инициализировать один раз на всю программу, то это еще один синглтон?

Сейчас QSettings создается на каждый вызов функций из settings.cpp. А мог бы создаватся только на инстанс объекта Settings. Для примера, было

ui->spinBoxTreshold->setValue(getTreshold());
ui->spinBoxDUP->setValue(getDUPChannelSettings());
могло бы быть
Settings settings;
ui->spinBoxTreshold->setValue(settings.getTreshold());
ui->spinBoxDUP->setValue(settings.getDUPChannelSettings());
Это так же поможет исправить такой код
QString datadir = "Settings";
if(!QDir(datadir).exists()){
    QDir().mkdir(datadir);
}
QSettings settings((datadir.toStdString() + "\\settings.ini").c_str(), QSettings::IniFormat);
bool ok;
auto PID = QString("0x" + ui->lineEditPID->text()).toInt(&ok, 16);
auto VID = QString("0x" + ui->lineEditVID->text()).toInt(&ok, 16);
settings.setValue("PID", PID);
settings.setValue("VID", VID);
в
Settings settings;
settings.setPID(PID);
settings.setVID(VID);

Txe
()

говно переделывай

anonymous
()

const'ов добавь

-    QString datadir("Data");
+    const QString datadir("Data");

и т.д.

fluorite ★★★★★
()

Ты никто. Развиваться можно только в команде профессионалов. Причем каких-то особых навыков для этого иметь не нужно. Забудь про всякие «мы ищем таланты», «хороших специалистов не найти» и т.д. - это все чушь. Можно взять любого школьника с улицы и сделать из него профи.

Так что поднимай жопу с дивана и иди куда-нибудь в сторону реальных проектов.

anonymous
()
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.