Вхід і реєстрація
Навчання · 26 серпня 2017, 10:24 1539
First Last

Code review небольшой программы

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

Программа не большая. Заранее спасибо :)

P.s: calc — первый проект на крестах(то есть, на С++ сижу 2 месяца (По факту еще меньше, так как около месяца на Qt сидел...) ), так что, мог в проекте допускать «много кода» вместо одной строчки и все такое.

Ссылка

Теми: code review
👍ПодобаєтьсяСподобалось0
До обраногоВ обраному0
LinkedIn
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Яна Костюк Junior Java Engineer – CloudMade 27.08.2017 21:42

а чего бы просто не назвать нормально переменные чтоб не писать комменты к ним?

// 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);

Відповісти
Підтримати
De Money 26.08.2017 11:45

Навіщо параметри закоментовано у хедерах? Таке роблять наскільки я знаю щоб не базікало про 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() тому питання чому С++ взагалі. Загалом наче програмувати вмієте) До кінця не продивився, міг щось пропустити.

Відповісти
Підтримати
First Last 26.08.2017 12:58

Параметры — после Qt стал так писать.

Про память — спасибо, гляну.

Насчет конструктора в Core не понял, там ведь только вызов метода.

Про JSON — знаю, не совсем то :) Не хотел писать на сокетах(или что там) запрос.
Насчет system — не совсем много, но без этого было бы куда сложнее :)

Спасибо.

Відповісти
Підтримати
De Money
Jorzchyk Pushysty 26.08.2017 13:26

Там другая проблема с 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;
Відповісти
Підтримати
First Last
First Last 26.08.2017 13:33

Спасибо, подумаю что можно сделать :)

Відповісти
Підтримати
Jorzchyk Pushysty
De Money 27.08.2017 00:54
Насчет конструктора в Core не понял, там ведь только вызов метода.

І цей метод робить УСЕ.
Конструктор за визначенням потрібен для ініціалізації. Це — не ініціалізація. Не очевидно, що відбувається. І якщо клас Core зробити глобальною змінною — у вас усе буде відбуватися поза main. Тільки от тоді про порядок ініціалізації глобалов непогано було б знати, бо вони якісь можуть існувати.

Потім, в С++ у конструкторі є проблеми з ініціалізацією віртуальних методів (питання для співбесіди «як зробити, щоб випало віконце pure virtual function call»). Тобто, якщо якимось чином там з’являться віртаульні методи, то з ними можуть бути проблеми. Воно працює, але це — поганий стиль.

Відповісти
Підтримати
First Last
George 26.08.2017 10:59 
if (1 == result)
    {
        cout << "Enter name for file" << endl;

        return;
    }

    if (2 == result)
    {
        cout << "File not exists" << endl;

        return;
    }

В умовах треба змінну і цифру, з якою порівнюєш, місцями змінити

if (result == 1)
Відповісти
Підтримати
George 26.08.2017 11:03
3 == vsv.size() ? stoi(vsv[2])

Тут теж.
На таке реально боляче дивитися.

Відповісти
Підтримати
George
Nikola Utiuzhnikov Кодировщик 26.08.2017 12:26

В стандартах оформления кода некоторых компаний прямо записано, что в коде надо использовать условия Йоды.

Відповісти
Підтримати
George
Dmytro Shchurov Frontend/React Developer в KAJO s.r.o. 26.08.2017 12:36

А якщо хтось помилиться та напише if (result = 1)? Сiшники не дарма пишуть const == var

Відповісти
Підтримати
George
Jorzchyk Pushysty 26.08.2017 12:49 
warning: suggest parentheses around assignment used as truth value
Сiшники не дарма пишуть const == var

if((result = 1)) або if(var == const).

Відповісти
Підтримати
Dmytro Shchurov
First Last 26.08.2017 12:53

Я давно так пишу, спасибо, мне вполне нормально :)
А вообще, я начал так писать после ссылка вот этого.

> Если писать не

// ПЛОХО?
if (x == 1)

а

// ХОРОШО?
if (1 == x)

то вы можете не бояться опечатать и написать «=» вместо «==». В хорошем варианте программа просто не скомпилится, если вы допустите такую опечатку.

Как-то и не задумывался о том, что кому-то и не нравится такой «стиль».

Відповісти
Підтримати
George
George 26.08.2017 13:38

Я просто навіть і не здогадувався, що такий стиль — норма.

Відповісти
Підтримати
First Last
Олексій Пєніє 26.08.2017 23:32

Все що заважає логічно мислити — генерує помилки та дуже заважає їх бачити. Тому йода-стайл створює більшу проблему, аніж вирішує.

Відповісти
Підтримати
First Last
Олексій Пєніє 26.08.2017 23:28

Такий запис небажаний, бо не наслідує людську логіку.
Або 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» дуже рекомендується обробляти через виключення. Бо цей код заважає розумінню основної логіки, засмічує його типовим бла-бла-бла, це заважає знаходити помилки..

Відповісти
Підтримати
George

Підписатись на коментарі

Не підписуватись
Скористайтесь акаунтом
×
з умовами використання сайту і політикою конфіденційності.