Code review небольшой программы
Привет, сделал небольшой велосипед (поэтому, камни не кидайте, пожалуйста. Нужна была практика и моя фантазия не особо хороша), закинул на гитхаб и был бы благодарен, если бы кто-то помог в плане советов: ошибки в коде, реализация в другом виде (более удачном) и т.п.
Программа не большая. Заранее спасибо :)
P.s: calc — первый проект на крестах(то есть, на С++ сижу 2 месяца (По факту еще меньше, так как около месяца на Qt сидел...) ), так что, мог в проекте допускать «много кода» вместо одной строчки и все такое.
19 коментарів
Додати коментар Підписатись на коментаріВідписатись від коментаріва чего бы просто не назвать нормально переменные чтоб не писать комменты к ним?
// vsv = vector shinda version, vgv — vector github version
auto vsv = json.split(sVersion, ’.’);
auto vgv = json.split(gVersion, ’.’);
// // a — shinda version ,b — github version
Versions a(std::stoi(vsv[0]), 2 <= vsv.size() ? std::stoi(vsv[1]) : 0, 3 == vsv.size() ? std::stoi(vsv[2]) : 0);
Versions b(std::stoi(vgv[0]), 2 <= vgv.size() ? std::stoi(vgv[1]) : 0, 3 == vgv.size() ? std::stoi(vgv[2]) : 0);
codereview.stackexchange.com
Спасибо
Навіщо параметри закоментовано у хедерах? Таке роблять наскільки я знаю щоб не базікало про unused parameter. В нас би мабуть clangtidy не пустив би через різні імена у хедері та срр.
У Core reader, json та github легко робляться не вказівниками і маємо простіший конструктор та дефолтний деструктор. General advice — avoid (explicit) new. У вас пам’ять здається тече на
auto checker = new Checker;
Бо не бачу delete. Якщо можна просто на стеку створити щось — створюйте на стеку (не можна для того що багато стеку займе, це дуже рідко, або жити повинно довше).
Не робіть основну роботу у конструкторі (як у Core). Він не для того, він повинен бути якомога простішим. Можна взагалі main() пустим лишати, але не треба.
Avoid using namespace std; По причинах, по яких існує namespace. Воно коротке. Якщо потрібен довгий страшний тип — напишіть typedef.
JSON якийсь не json’істий (ваш shinda.json — не valid json). І тому виникає питання що це все таке. Взяли curl — візміть для json теж.
Багато робиться на system() тому питання чому С++ взагалі. Загалом наче програмувати вмієте) До кінця не продивився, міг щось пропустити.
Параметры — после Qt стал так писать.
Про память — спасибо, гляну.
Насчет конструктора в Core не понял, там ведь только вызов метода.
Про JSON — знаю, не совсем то :) Не хотел писать на сокетах(или что там) запрос.
Насчет system — не совсем много, но без этого было бы куда сложнее :)
Спасибо.
Там другая проблема с system.
int FileSystem::removeDir(const std::string &path) { auto s = "sudo rm -rf " + path; system(s.c_str()); }sudo нельзя так вызывать. Просто «rm -fr ...». При необходимости юзер сделает sudo shinda.
system() в таком контексте лучше заменить на подходящую exec(). Или иначе проверять на проблелы, спецсимволы etc но в этом нет смысла.
int pid, status; if((pid = fork()) < 0) /* error */ if(pid == 0) { if(execl("rm", "-fr", path, NULL)) /* error */ _exit(0); } if(waitpid(pid, &status, 0)) /* error */ return status;Спасибо, подумаю что можно сделать :)
І цей метод робить УСЕ.
Конструктор за визначенням потрібен для ініціалізації. Це — не ініціалізація. Не очевидно, що відбувається. І якщо клас Core зробити глобальною змінною — у вас усе буде відбуватися поза main. Тільки от тоді про порядок ініціалізації глобалов непогано було б знати, бо вони якісь можуть існувати.
Потім, в С++ у конструкторі є проблеми з ініціалізацією віртуальних методів (питання для співбесіди «як зробити, щоб випало віконце pure virtual function call»). Тобто, якщо якимось чином там з’являться віртаульні методи, то з ними можуть бути проблеми. Воно працює, але це — поганий стиль.
if (1 == result) { cout << "Enter name for file" << endl; return; } if (2 == result) { cout << "File not exists" << endl; return; }В умовах треба змінну і цифру, з якою порівнюєш, місцями змінити
Тут теж.
На таке реально боляче дивитися.
en.wikipedia.org/wiki/Yoda_conditions
Зачем?
В стандартах оформления кода некоторых компаний прямо записано, что в коде надо использовать условия Йоды.
А якщо хтось помилиться та напише if (result = 1)? Сiшники не дарма пишуть const == var
if((result = 1)) або if(var == const).
Я давно так пишу, спасибо, мне вполне нормально :)
А вообще, я начал так писать после ссылка вот этого.
> Если писать не
// ПЛОХО?
if (x == 1)
а
// ХОРОШО?
if (1 == x)
то вы можете не бояться опечатать и написать «=» вместо «==». В хорошем варианте программа просто не скомпилится, если вы допустите такую опечатку.
Как-то и не задумывался о том, что кому-то и не нравится такой «стиль».
Я просто навіть і не здогадувався, що такий стиль — норма.
Все що заважає логічно мислити — генерує помилки та дуже заважає їх бачити. Тому йода-стайл створює більшу проблему, аніж вирішує.
Такий запис небажаний, бо не наслідує людську логіку.
Або switch, якщо селектор має багато передбачуваних значень, або (найчастіше)
if(result <0){ // спочатку перевірити на помилки, щоб легше читати наступний код не думаючи про них
// do something with it;
}else if(result == 1){ // найбільш очікуваний результат
// perform main logic here;
}else if(result == 2){ // «Магічні» цифри — серйозна стилістична помилка.
// В бойовому коді рекомендується мати іменовані константи, бо що таке 2 — незрозуміло.
}else{ // Якщо ти дійсно чогось іншого очікуєш і не знаєш що прилетить
}
Тобто, if — else if — else читаються дуже швидко, типовий розбір логіки, навіть самої нетривіальної. А от декілька іf означає, що код має пройти їх всі, і теоретично можлива ситуація (особливо у мовах без суворої типізації) коли виконається одразу декілька умов.
Взагалі-то проблеми типу «File not exists» дуже рекомендується обробляти через виключення. Бо цей код заважає розумінню основної логіки, засмічує його типовим бла-бла-бла, це заважає знаходити помилки..