Отрецензируйте, пожалуйста, код

Это не KISS и не имеет вменяемых оправданий, кроме лени разработчика

суть в том, что мне не нужно оправдываться. Под «простая программа» не имелось ввиду KISS, а скорее simple by design. Я не предполагал в дальнейшем расширение функционала программы.

Когда тебе в ТЗ напишут написать helloworld, ты тоже будешь сплитить текст в отдельный хедер или заниматься мастурбацией как ребята из GNU Hello?

Кнута

не читал, и даже в глаза не видел. Что там конкретно там стоит посмотреть? А то я даже K&R забросил еле дочитав до 1/3

проверял только лишь readme.md

Он не намного лучше.

Буду писать как положено, в зависимости от ситуации и того, что я хочу получить. Если показывать школьникам, то наиболее просто и примитивно, если для работодателя, то посерьёзнее. А если я хочу критики, то буду писать точно так же, как ребята из GNU Hello. Ты пришел за критикой, а пишешь так, как будто тебе надо объяснять нубам, что такое функция и как её вызвать. А большое количество мелких недочётов, вроде магических цифр, говорит о том, что опыта на объяснения у тебя нет и ты сам в них нуждаешься. Идея твоя похвальна, но запили нормальный, полноценный калькулятор, а потом проси критику.

И да, ты уже оправдываешься своим постом, а это значит, что ты всё понял, просто не хочешь этого признавать.

set(SOURCE_FILES
    main.c buttons.h)

add_executable(n0calc ${SOURCE_FILES})

buttons.h что там делает?

place_and_bind_button'ов не много у тебя ;) ? Не лучше было бы организовать так, чтобы кнопки не были захардкожены настолько?

Ну а так, да, ты зря все в одну кучу положил

Спасибо за развернутый ответ!

В заголовочных файлах переменные не определяют, и вообще непонятно зачем этот файл.

чтобы отбросить часть кода, связанного с кнопками долой из основного кода программы

Если функция не меняет строку - делай const char *.

Кстати, можно где-то почитать об этой «традиции»?

while идиоматически переписывается на for

while проще читается...

Не надо в одну функцию мешать GUI и вычисление, даже в хелловорлде.
button_exec - сплошной копипаст, можно без него. place_and_bind_button - тоже, лучше меняющиеся данные занести в массив структур и сделать по нему цикл.

Да, я думал о какой-то замене.

мне такие конструкции показались излишне затрудняющими чтение, громоздкими, и трудными в работе для процесса разработки (последнее уже оправдало себя)

Или похитрее логику найти, чтоб не ломать голову в заборе «3, 0, 1, 1».

Вид формы можно вообще выносить в отдельный файл, можно даже отделить от бинарника и держать в виде xml. Но по личным соображениям мне очень не нравится это делать.

static double result - не очень уместный static. Лучше результат хранить вне функции, и передавать если надо.

нигде кроме той функции переменная не использовалась. Я тыкнул туда ключ. слово static лишь по той причине, что анализатор кода на него ругался.

Магические числа: 24, 30, 31.

С 30 и 31 полностью согласен, исправлю в скором времени. С 24 особенный случай - там корректнее прокомментировать что это такое, чем выносить в #define

{«Malformed expression at 1 operator!»} - скобки лишние.

Поставил чисто для себя. Ибо выражение «data» в Си возвращает указатель на неизменяемую область данных, я хотел явно видеть что это изменяемая строка.

Некоторые функции static, некоторые нет.

Я когда начинал писать программу вообще не видел смысла ставить куда-либо ключевое слово static перед объявлением функций.
Но документация для gtk указыват, что для функций, работающих с тулкитом следует использовать static. Те функции, которые БЕЗ static - они вообще независимы. Ни от чего. Я их могу скопипастить в любую другую программу и они будут работать.

Отрицательные индексы массива - нарушение стандарта.

квадратные скобки - это препроцессорное преобразование

arr[1]; // *(arr+1);

Разбор операндов копипастится.

откуда?

sscanf скорее всего хватило бы.

Один хороший знакомый посоветовал избегать sscanf(). Ну а вообще да, если бы я отдельно писал валидацию, был бы смысл влепить sscanf() и не парить мозг

buttons.h что там делает?

clion автоматически туда его влепил. Работает - ну и хрен с ним

place_and_bind_button'ов не много у тебя ;) ? Не лучше было бы организовать так, чтобы кнопки не были захардкожены настолько?

XML-фобия

Где грамматика выражений?

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

И да, ты уже оправдываешься своим постом, а это значит, что ты всё понял, просто не хочешь этого признавать.

мы, видимо, друг друга не поняли. Во всех местах в которых я откровенно зафейлил я откровенно признал свою неправоту (см. внимательно тему). Если у тебя понятие «оправдание» какое-то другое, пожалуйста, определи его тут

если для работодателя, то посерьёзнее

если для нанимателя, то посерьёзнее

пофиксил

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

Где грамматика выражений?

Отсутствует, я её и не собирался писать, о чем явно сообщил в описании к программе

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

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

The snprintf() function shall be equivalent to sprintf(), with the addition of the n argument which states the size of the buffer referred to by s. If n is zero, nothing shall be written and s may be a null pointer. Otherwise, output bytes beyond the n-1st shall be discarded instead of being written to the array, and a null byte is written at the end of the bytes actually written into the array.

http://pubs.opengroup.org/onlinepubs/009695399/functions/fprintf.html

n=31, n-1st это 30-й по счету, 29-й по индексу. 31-й по счету, т.е. 30-й по индексу будет терминатором. Другими словами snprintf всегда терминирует результат, если размер буфера > 0.

нет автомата состояний

не везде нужен :) Был в одном бизнес-фреймворке автомат с кучей невнятных состояний, выставленных наружу для «удобства» (кто бы ими еще пользовался). Когда пионеру поручили писать тестилку для этого фреймворка... Как он бедный мучался, пытаясь привязать пачку состояний к событияем тестов, хотя там было всего джва нужных состояния: реквест и респонс. Ради которых городить автомат вообще смысла не было. Все остальное — бизнес правила, от биржи к бирже разные, которых настолько много, что ты крякнешь писать для них автомат :)

вот это был калькулятор, проект из этак десятка классов, где четкое разделение.

калькулятор в K & R мне милее. А вотэто «я когда то писал калькулятор с поддержкой систем счисления от двоичной до шестнацетиричной. » — для каждой разве нужен отдельный класс? :) Линейной комбинации степеней основания недостаточно?

n=31, n-1st это 30-й по счету, 29-й по индексу. 31-й по счету, т.е. 30-й по индексу будет терминатором. Другими словами snprintf всегда терминирует результат, если размер буфера > 0.

благодарю, поправлю! я почему-то функции с буквой n внутри запомнил как те, что «не пишут в буфер больше чем n». Подумал, что имелось ввиду всё, кроме нуль терминатора.

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

По коду:

• парсера нет, уже б я не знаю, консоль дергал с bc
• mvc или observer pattern'а нет, отсутствие их в gui приведет к катастрофе при чуть более жирном приложении. В итоге у тебя gui намертво переплетено с логикой.
• вспомогательные и утилитарные функции перемешаны с функциями отвечающими за гуйню: strchrcount, strcharsreplace, ... (найди остальные)
• magic numbers: if (strchrcount(field, '.') > 2), malformed_expression_at_operator[24] = *operator; char result_str[31];

Но документация для gtk указыват, что для функций, работающих с тулкитом следует использовать static. Те функции, которые БЕЗ static - они вообще независимы. Ни от чего. Я их могу скопипастить в любую другую программу и они будут работать.

4.2 и заблуждения. Гтк пофиг, какой у функции storage class, т.к. это просто адрес с определенным abi. Сами статики видны только в текущем translation unit'е. Видны значит существует соответствие между символом и адресом.

В итоге у тебя gui намертво переплетено с логикой.

Вот мы вроде все неглупые люди, но прчему-то думаем, что в будущем отупеем и вместо развязки гуя с логикой начнем сверху хуяк-хуяк и в продакшн. Мое мнение — все сделано правильно, за исключением мелких косяков, и код читается абсолютно прямолинейно. Если нужно усложнить, то тот кто правит должен включать свою башку и смотреть по факту, нужен ли рефакторинг. Иначе получается, что мы заранее вслепую делаем работу, которая еще хз подойдет под будущие нужды или нет, а чтение и понимание смысла упадет сразу и сильно. Не надо так.

Вот если бы оп озвучил перспективы и цели, типа встраивание там, ооп-совместимость, использование без гуя, etc., то можно было бы ему накидать, что контроллера нету, окно прибито, филды инлайном и все в этом роде. А так все норм, это же hellocalc.

mvc или observer pattern'а нет, отсутствие их в gui приведет к катастрофе при чуть более жирном приложении. В итоге у тебя gui намертво переплетено с логикой.

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

Хотя, если вы по принципу проектирования, не требующему переписывания кода разработку ведете, то нормально.

Если бы писал для разработки с кем-то еще, в первую очередь посоветовался с этим человеком.

Вот у тебя как раз не читаемо.

как раз хочу выпрямить руки с самого начала чтобы потом не вошло в привычку

4.2 и заблуждения. Гтк пофиг, какой у функции storage class, т.к. это просто адрес с определенным abi. Сами статики видны только в текущем translation unit'е. Видны значит существует соответствие между символом и адресом.

в таком случае, пойду почитаю внимательнее что такое static

Если человек учится, то надо учиться делать как положено :)

Типичный пример (псевдокод):
Так у ТС (примерно).

static void validate_and_calc() {
  ....
  GtkTextIter start_iter, end_iter;
  gtk_text_buffer_get_start_iter (buffer, &start_iter);
  gtk_text_buffer_get_end_iter (buffer, &end_iter);
  ...

  gtk_text_buffer_set_text(buffer, result_str, (gint) strlen(result_str));
}

static struct EvalResult validate_and_calc(char* expr, size_t exprLen) {
  ..
  struct EvalResult result;
  result.status = ... ; // ok, fail (error code for example)
  result.evalResult = ....; // result of evaluation, if succeeded
  return result;
}

Сложнее? - Нет. Читается хуже? - Нет. Зато есть четкое разделение Gui и логики.

Писать прикладное ПО на plain C - сразу кол.

Недавно развлекался с Makefile, разросшимся на 10к+ строк, часть из которых копипаст, часть уже оторвана от реальности. А ведь начиналось все с простой билдилки и пускалки, и вроде неглупые люди делали.

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

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

Тогда привыкай графику отделять от логики, не объединять разные по смыслу вещи в одну функцию/объект во имя лени, писать все объявления в хедерах, а реализацию в c файлах, комментировать код, всякие библиотеки или что-то, где много логики лучше вообще документировать (хотя бы у каждой функции, где больше 5 строк или нетривиальная логика) писать doxygen комментарий, что она делает и какой смысл несут аргументы.

Я имел дело с приложением, которое из простого диалога со списком и парой кнопочек разрослось в ужас на более чем 50000 строк, которое рефакторить просто не возможно... потому что весь рефакторинг выльется в переписывание с нуля фактически (что надо сразу и начинать делать).

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

Тогда привыкай графику отделять от логики, не объединять разные по смыслу вещи в одну функцию/объект во имя лени, писать все объявления в хедерах, а реализацию в c файлах, комментировать код, всякие библиотеки или что-то, где много логики лучше вообще документировать (хотя бы у каждой функции, где больше 5 строк или нетривиальная логика) писать doxygen комментарий, что она делает и какой смысл несут аргументы.

Спасибо. В идеале я так и хочу.

Не понятно только одно.

писать все объявления в хедерах, а реализацию в c файлах

зачем это делать?

3) Написать аудиоплеер

(остряк mode on)

#!/bin/bash
vlc "$@" || mplayer "$@" || for x in "$@"; do ffplay "$x"; done || cat "$@" > /dev/dsp

копипаст

Ну да, тот самый момент, когда надо задуматься :)

стартап

ой вей, какое горе. В стартапах еще и на разделение труда болт кладут от бедности: это они придумали devops и «fool-stack абизян» :)

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

Ога, а гугл придумал SRE, тоже от бедности.

Гугл придумал до*я велосипедов от NIH, но это офигеть какая кулстори :)

Это был не совсем стартап. Это фирма, у которой был основной источник дохода, но они решили попробовать себя в разработке ПО. У них из разработки ПО ничего не вышло.

Начинается... Стартап не совсем стартап :) Нишмагли в диверсификацию бизнеса и бэкап-стратегии? ССЗБ, чо.

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

Не, фирма не загнулась, живет.

Но разработка ПО прибыли не приносит,

значит это не бизнес-проблема, а маниловщина руковоцтва :)

Проблема в том, что они 10 лет пишут то, что можно написать за пол года-год заново и по-человечески :)

Проще читать что у тебя там есть - одни объявления и комментарии что делает та или иная функция, больше ничего не нужно

разве для этого нет понятия «документация»? Плюс doxygen, и всё такое.

Просто когда я вижу жесть типа

int func();
int func() {
    //...
    return 1;
}

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

double doSomethingUseful1()
{
//коротко
}
double doGreateCalculations1(double a, ....)
{
//много всякой лабуды
}
int shortUsefulFunction()
{
//маленькая функция
}
double doSomethingUseful2()
{
//коротко
}
double doGreateCalculations2(double a, ....)
{
//много всякой лабуды
}

зачем это делать?

Чтобы всё было «модульно».

// a.c
#include "b.h"
#include "c.h"

void f() {
  b_doThings();
  c_doThings();
}

// b.h
void b_doThings();

// b.c
#include "b.h"

void b_doThings() { pruntf("b\n"); }

// c.h
void c_doThings();

// c.c
#include "c.h"

void c_doThings() { pruntf("c\n"); }

то, что можно написать за пол года-год заново и по-человечески :)

С задним-то умом и аджайлом еще ненене, это вовсе не эффекты «второй системы» :) Где написаные по аджайлу/скраму/канбану с няшными паттернами эвривер системы уровня операционок и БД? Нету! — только валяние «пруфов канцепта», из готовых кубиков (паттерны тоже кубики), которые внезапно попадают в продакшон почти в неизменном виде, с допилками потом, за «отдельные деньги», а в «естественно-вотерфольных» проектах с их сроками и бюджетами «микрооптимизации» типа модных вот щас методик и техник плюс-минус ни на что не влияют (и методики протухают, и концепции меняются, и технологии, и «ментальность» команды, вместе с человеками, а бизнес остается, как и легаси, которое переписывать — бизнес крякнет — и что-то делается в сроки на уровне соблюдения здравого смысла и с учтенными издержками), потому что... нельзя 9-ю беременными родить за месяц то, на что по технологии положено 9 месяцев (Брукс «Мифический человеко-месяц» про это писал: критический путь на сетевом графике остается критическим путем на сетевом графике, никакие модные методики на это не влияют, ничо не распаралелится на большее количество землекопов, и быстрее не выйдет, — никаких «пятилеток в 3 года», особенно если команда начинала не с этим багажом знаний, и состав и «ясность концепции» по дороге менялись). 10 лет пишут — либо потому что оно так и есть, а серебряной пули нет, либо потому что не очень надо (такое тоже бывает в реальном секторе, потому что отдача получится не сразу, а премию за экономию тоже хочется). Именно поэтому жырные проекты про внедреж SAPа в нефтянку, отраслевой тугаментооборот и прочее ERP — это чаще «тайм энд матириал» без гарантий в доморощеном подразделении типа «центор коньпетенцыии» (возможно, прикупленном со стороны вместе с командаме (раз)рабов), а не «фиксед прайс и дедлайны» на аутсорце в стартапах.

Нанять квалифицированных

Раньше-то у них пелена на глазах была :) А может, концепция/бюджет были несколько иные... 10 лет прошло.

переписать с нуля

Это вообще редко кто делает, если вообще что-то работает :) Жызненный цикол ситемы таки да, согласно документам может включать в себя пункт «прекращение эксплуатации», за ради внедрения новой системы. Но с точки зрения бухгалтерии — моральное устаревание вообще не повод выделять бабки :) Помню для допилок биржевого фреймворка под очередную «игровую площадку спекулянтов» нам запрещали говорить что «мы разрабатываем» — «мы конфигурируем»... Угу. На С++.

Это вы такое писал

#include<math.h>
#include <stdio.h>
#include <conio.h>
int main()
{
float a,b;
char ch;
scanf («%f%c%f»,&a,&ch,&b);
switch (ch)
{
case '+': printf(«=%g»,a+b); break;
case '-': printf(«=%g»,a-b); break;
case '*': printf(«=%g»,a*b); break;
case '/': if (b) printf(«=%g»,a/b); break;
case '%': printf(«=%g»,fmod(a,b)); break;
default: printf(«Input error!!!»);
}
getch();
}

или на FSM ?

https://github.com/xdevelnet/n0calc

такое говно на билдере в винде делается на 4 минуты.

На этот случай даже варнинг есть — публичная функция без форварда. Я в общем-то и статики в начале .c выписываю, потому что рандомные форварды в исходнике приводят к тому, что со временем их появляется больше одного и путаница.

Две кнопки для выхода не нужны.