среда, 20 июня 2018 г.

Критика кода

В стане моих "коллег" по цеху появилась тема с критикой кода который написал я. Я очень рад, что мой проект и код вызывают такой интерес и не оставляет равнодушным. Но, к сожалению, не могу пройти мимо и не прокомментировать некоторые вещи. Просто потому что иначе вы не сможете узнать альтернативную точку зрения. Этот пост на программистскую тему, поэтому если вам это не интересно, вы можете смело его пропустить.

Критика кода это самое обычное дело для программиста. И самое главное не принимать ее слишком близко к сердцу. Иначе получится как то так:


Давайте же разберем так называемую критику озвученную "экспертами" в программировании и большими знатоками этого дела. Я буду приводить цитату без перевода с своими комментариями.
Douglas
I also recommend to follow the Qt widget naming convention… like using a combobox name firstDayComboBox not comboBoxFirstDay. The Valentina code was done using the later and IMO making it harder to read.
Вообще то соглашение об именовании переменных может быть и своим. Главное в этом деле придерживаться одного стиля. Как говорят лучше плохой стиль, чем его полное отсутствие. Немаловажным фактором является мой родной язык. Как известно в нем принят обратный английскому порядок слов. Что означает, что выбранное мной написание не кажется мне странным. А вот для автора, который является англоязычным борцом за чистоту английского языка, мозг ломается. В таком случае я признаю проблему. Но все же, как единоличный автор, оставляю за собой право писать как мне удобней. Видилите, что бы вам не говорили, но практически весь код проекта написал лично я. Все остальные прилипалы только и кричат, что они тоже сделали вклад. Когда мы работали вместе, у них не было вопросов к именам, соглашениям и ко всему остальному. Я был очень хорошим программистом, если не сказать гениальным. Я все же считаю, что поменять порядок следовало бы, но вот на данном этапе это потребует некоторого времени, ввиду того, что переменных очень много.
Douglas
Also use full names - do not abbreviate. Here’s a BAD name example used in the code… GetPPath()… well what the hell is PPath? Printer Path? Pattern Path? Piece Path?
Люди любят сокращать. Особенно если они знают, что это означает. Когда ты делаешь что то сам, не всегда получается сделать читаемо для стороннего читателя. Конечно в первую очередь я думал о себе, смогу ли я понять, что означает это имя. Но штука в том, что я уже исправил имя на полное  GetPatternPath(). Это заняло меньше минуты. И все. Автор, который показывает кое какие знания в программировании, вместо жалоб должен был предложить это исправление сам. Даже если он бы не правильно угадал, только так нужно поднимать такие вопросы. А то ведет себе как полный дилетант. Мало того, что прячет от сообщества свою приватную версию программы, так и еще выступает с критикой. Его код мне не посмотреть - не очень честно.
Douglas
BTW… you also have to have an understanding of how the schema works to add any new functionality to the program. The schema has to have elements added to identify a new object and it’s properties that are used to read/write the pattern, measurement, & label files. The schema is also used in another part of the program to validate the program version used… that is if you add a new 3 point line, an older program version can’t open & read the new 3 point line object.
Oh… and you also have to add the visualization stuff for the added tool… which means you need to understand how to work with the QGrahicsView and QGraphicsScene class functions. Also the new tool will need a UI dialog and you will need to know how to fill the various comboboxes with the object data… as well as the tool option creator in the PropertyExplorer section to be able to edit the object’s properties.
Almost forgot… you also have to setup undo/redo functions and push them to the undo stack.
Sounding rather daunting eh?
Все верно, коротко описаны все необходимые действия для того чтобы заработал инструмент. Вот ему не понравилось, что инструменту нужен: диалог, класс для создания объекта, код для парсинга файла, изменения в формат файла, визуализация (подсветка), код для отмены создания и редактирования, код для показа информации в боковом меню. Но постойте. Как говорится здесь же ничего лишнего. Разве не это принцип идеальности? Все это добавлено практическим путем. Что же тогда господину не нравится? А видимо то, что ему сложно во всем этом разобраться. Пока я за него это делал, все было хорошо, а тут нужно свое драгоценное время тратить. Я то хочу две строчки и в дамках. Критикуешь - предложи схему получше, научи меня как. И кодом, кодом, а не языком. Ты путаешь професси.
Douglas
After 5 months or so of plowing through the code - I agree. I find it tedious. It’s like let’s throw a bunch of stuff against the wall and see what sticks. There’s code that just seems redundant - for ex: the (misnamed) tool option dialogs use the Creator UI, while the Tool Options docker uses the PropertyExplorer library…??? IMO pick one way or the other.
Автор какой то странный. А он вообще работал с чужим кодом хоть когда то? Вообщем это очень сложно понять такой объем кода вот так легко. Порой даже с хорошей документацией и более мение стандартной структурой сложно уловить все нюансы. Чувствуешь, что по частям ты понимаешь код, а вот всю картину нет. Вообщем все здесь понятно. Опять критика в стиле все не так. Идем дальше.
Douglas 
Also variables, functions, classes, etc are sometimes poorly named (obviously due to the language barrier) making things confusing, and overall the code does not always follow a standard naming & style convention. I also find the whole folder structure horrendous. Going more than 3 folder levels down is BAD… not to mention there are resource files in no less than 3 separate places? Why? Unfortunately moving stuff around is not easy… you have to change any references, re commit, and edit .pro & .pri files using Creator or manually.
Об именах мы уже поговорили. Вывод один. Не нравится, предлагайте изменения. А если код пишу только я, то мне ок. Больше 3 уровней вложености директорий. Мда. Я предпочитаю видеть файлы сгруппированными по группах. А директории как раз очень хорошо с этим справляются. Это очень смахивает на спор теоретика с практиком. От куда цифра 3? Кто его знает. Но зато это очень красивая цифра. Неправда ли? Есть такой принцип в философии Toyota. Хорошая организация это та при который, если попросить что то найти, работник быстро это отыщет. Я знаю где что у меня лежит. Для других нужна просто небольшая инструкция. Ну так попросите. Уж извините, но у меня тоже нету времени описывать проект в таких подробностях. Тут либо новые функции, либо документация. А желающих писать код раз, два и обчелся. Файлов же много. В трех уровнях такое уместить можно, но вот как бы мне кажется видеть под 30 файлов (если не больше) в каждой директории тоже не очень комфортно будет. И опять мы возвращаемся к очень интересному моменту. Структура нам не нравится, но вот делать мы ничего с этим не хотим. А почему? Ведь если сделать изменения у себя, то становится очень сложно потом переносить изменения с публичной версии, моей или моих коллег. А вот почему он тогда не хочет сделать такие изменения в публичной версии. Это он пускай сам отвечает. Отмазка о большом количестве изменений не принимается. Теперь они начали мычать о объемах работы которые я раньше молча делал.
Douglas 
I also find it frustrating that many of the components of the program are not fully thought out from the start… which leads to “let’s throw more crap at the wall to make it work.” I ran Doxygen to get a handle on the classes and found the same thing with some derived classes being more than 3-4 levels deep - which means it’s harder to read & follow the code, and the base class probably wasn’t thought out well enough.
Покажи мне идеальную вещь и я найду в ней изъяны. Это один из главных абзацев побудивших меня прокомментировать. Я не скрываю тот факт, что я не знаю все об пошиве и создание выкроек. Я познаю тему в процессе. Я приверженец эволюционной модели. Это совсем не означает, что я не могу или не хочу продумывать вещи наперед. Вот только я могу построить вещи которые я понимаю. И как раз на этом этапе ваша помощь очень незаменима. Тестирование, отзывы, критика, все это дополняет картину любого мастера. Без них я создаю свою картину и не всегда она правильна. Это похоже на ограниченность обзора из за тумана. Да, в эволюционной модели есть недостатки. То, за что меня и критикуют. То, по чему многие виды на планете вымерли. Принцип если бы мы знали сразу, то тогда сделали бы по другому. А где же взять эти знания? Только опытным путем, а значит опять ошибки или лучше вообще не браться, как нам услужливо намекает автор? Эволюцию называют слепым часовщиком, красивое название не правда ли? Я конечно не эволюция, но тоже пытаюсь следовать принципам которые позволят не допустить невозможности дальнейшего развития. Есть такие вещи как, соглашение об именовании, статические анализаторы кода, предупреждения компилятора, тесты, модульность кода, рефакторинг, рекомендации к организации структуры кода и многое другое. Пока что я не чувствую ситуации при которой нужны были бы кардинальные изменения в архитектуре для того чтобы продолжить развитие. И это хороший знак. Пока что любые изменения досягаемы за разумное время. Ну а насчет классов. Ну опять тоже ж самое. Поищите картинки по запросу qt classes hierarchy и сами посчитайте как много уровней у них. 3-4? Нет. Намного больше. Вообщем абстракция. Если и хочешь добится такого результата, хотя бы посмотри как это сделать и почему оно сразу не так. Авось и я перейму твой опыт.
Douglas 
It’s almost like Roman made it intentionally hard to add or make changes to the code.
Мои вы хорошие. А вам не приходило в голову, что я дальше пишу код?! Это же я сам себе палок в колеса понаставлял. Как же так выходит, что я могу эффективно один писать и поддерживать код, а вы, такие эксперты с красоты, не можете. Если не шмогли (в слове нету ошибки), так уже бы молчали. Я же сразу говорил, вы не уважаете мой труд как программиста, лезли со своими советами ни черта не смысля в этом. Советы то разумные порой, но вот не в такой форме, пылу слишком много, абстракции. Покажи мне свой код. Вот какой принцип должен быть у программистов. Если на словах не договорится, стань и утри мне нос своим кодом. Вложись своим временем и докажи. А так это трепня ниочем.
slspencer 
In hindsight, I tend to agree with you. What we need is a complete rebuild. This code is really a mess, and it crashes alot. The Qt company said that it would provide an analysis and documentation of how it is built, the architecture, and use of widgets for $8000. Which is a deal. But I don’t have $8k.
Это сообщество любит теории заговоров. И это говорит лидер проекта, сооснователь. Человек называющий себя программистом (якобы знает Python). Публично заявляет, что не будет учить С++. Какой ты тогда лидер проекта? У тебя видно много денег чтобы спонсировать других. Судя по коментариях не похоже. Задумайтесь. И за этим человеком вы идете. Все кто с ней познакомился говорят, что она хороший, отзывчивый, открытый человек. Но совсем не программист, а менеджер по пиару. У меня сложилось впечатление, что если она и писала код, то это было очень давно. Ну не думает она как инженер. И оно такое не у меня одного. Но что это я перешел на личности. Все это для того чтобы в очередной раз показать, что с такими людьми у проекта нету будущего. Главный вопрос - кто будет писать код? Даже не пробуйте спрашивать у них напрямую. Не тратьте свое время. Вам либо солгут, либо скажут, что вы тролль, провокатор. Полное переписывание. Ну что же удачи. Даже у больших компаний как Microsoft не всегда выходило переписывать продукты с нуля. Это почти всегда очень плохая идея. Скажите мне зачем здоровому самодостаточному сообществу нужна помощь компании "Qt company" для того чтобы разобраться в том как программа работает? Вопрос риторический.

kmf
I am capable of making “patches” to spagetti code, but not willing to put those up for public use because of the daunting task of regression testing to insure that I did not break something. 
Спагетти я люблю.

Вообщем делайте выводы сами. Я все лишь озвучил свою позицию. Я с радостью приму конструктивную критику в форме вот тут не правильно, нужно сделать вот так. Код прилагается. Тогда можно дискутировать. Особенно если вы программист и показываете, что знаете как сделать лучше.

2 комментария:

  1. :))))). Ну вот, Вы и дождались какого-то шевеления с их стороны, пусть пока в виде профессионального трёпа. Но всё-таки они пытаются сдвинуться с мертвой точки. А у Вас сейчас есть над кем поприкалываться. А то BORING ему видите ли, BORING. :)))

    ОтветитьУдалить
    Ответы
    1. Это скорее похоже на попытки завести двигатель, а ключей то нету.

      Удалить