Jump to content

    
haker_fox

Как у вас решается вопрос "ревью кода" на работе

Recommended Posts

Добрый день! Собственно говоря весь вопрос в теме. Лишь некоторые пояснения требуются. С одной стороны "ревью" - штука замечательная. С другой стороны на своей работе наблюдаю такую картину: начальник отдела часто изучит какую-то новую фичу языка Си++ и начинает её активно проталкивать. Причём, есть ощущение, что делается это не с глубокими пониманием смысла. Одна из моих недавних тем посвящена этому. Сейчас человек настоятельно рекомендует использовать constexpr вместо static const.

Например, был код

static const auto MY_CONST = 10;

Его "следует" заменить на

static constexpr auto MY_CONST = 10;

При этом не учитывается, что простые типы данных не вызывают конструктора по умолчанию, и такая замена ничего не даёт.

 

Другой пример, у меня был класс с полностью статическими методами. Мне его настоятельно рекомендуют заменить на namespace с функциями. Но не учитывается, что в таком namespace неудобно объявлять переменные (члены класса).

 

В общем вопрос следующий: как у вас на работе обстоит с этим дело? Ведь необходимо, чтобы человек не только умел "красиво" писать код, но при этом код этот транслировался в красивый машинный код. А этот фактор не учитывается, т.к. человек не работает с железом каждый день, а пишет более высокоуровневый код. Более того мне было недавно сказано, что зачем изучать регистры арма и как там всё устроено, когда нужно сначала изучить язык... Я признаю, что некоторых вещей в Си++ не знаю, да и не хочу особо знать, мне больше интересно заниматься железом. Но такое положение дел просто мешает работать: вместо программирования железа и периферии, разбора их нюансов и поведения, начинаешь украшать классы, что ИМХО не так важно. Я не говорю, что код нужно писать левой пяткой через правое ухо. Но то, что я наблюдаю больше похоже на попытку улучшить что-то без понимания вещей.

Share this post


Link to post
Share on other sites

Ну, если прораб начнет опытного каменщика учить как "правильно" класть кирпичную кладку, то будет закономерно послан на три буквы.

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

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

И потому он не станет соваться со своими "полезными" советами туда, куда не следует 

 

 

 

Share this post


Link to post
Share on other sites
19 минут назад, haker_fox сказал:

В общем вопрос следующий: как у вас на работе обстоит с этим дело?

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

ЗЫ. Есть вещи, когда работа идет коллегиально, тогда мы договариваемся, как все это сделать так, чтобы потом "поженить" все части и тут возможны компромиссы...

Edited by mantech

Share this post


Link to post
Share on other sites

Ваш вопрос ведь не относится к ревью кода.

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

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

Share this post


Link to post
Share on other sites
5 minutes ago, Forger said:

то будет закономерно послан на три буквы.

По понятным причинам это не наш метод)

5 minutes ago, Forger said:

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

Во-во. Он меня учит как программировать красиво. А то, что я полдня разбираюсь почему таймер не работает как хочется или почему возникает такая-то ошибка, не понимает.

6 minutes ago, Forger said:

И потому он не станет соваться со своими "полезными" советами туда, куда не следует

Эх...

6 minutes ago, mantech said:

когда указывают на чем писать и как это делать.

Я вот об этом же. Какая разница как писать? Ну, понятно, что ни как попало.

6 minutes ago, mantech said:

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

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

3 minutes ago, one_eight_seven said:

о чём вы говорите - это больше относится к разработке стиля кодирования, чем к ревью, как таковому.

Возможно. Наш начальник считает, что ревью это и стиль. Причём на стиль я бы сказал у него больший акцент. Что заставляет задуматься.

4 minutes ago, one_eight_seven said:

Ну и, судя по тому, что вы пишете, у вас не какая-то серьёзная продуктовая фирма,

Не стартап. Фирме уже более 25 лет. И давно и даже успешно выпускаем продукцию. Но эти придирки к коду... за...ют уже...

4 minutes ago, one_eight_seven said:

Если нет, меняйте начальника.

Чувствую, что такой начальник один - я сам)

Share this post


Link to post
Share on other sites

Самодурство начальника не имеет к ревью никакого отношения. 

Ревью это не сдача проекта заказчику. А конструктивный диалог между специалистами, который позволяет улучшить конечный результат.

Если единственный ревьюер - ваш руководитель, который не воспринимает никаких доводов и не хочет аргументировать свою позицию, то вариантов два:

1. Смириться 2. Сменить работу.

6 minutes ago, mantech said:

У нас очень просто - нужно написать так, чтоб это работало и надежно

Надежно работать может и такой код, в котором не разберётся никто, кроме автора. А это критично для больших долгоиграющих проектов.

Share this post


Link to post
Share on other sites
2 minutes ago, Corvus said:

Если единственный ревьюер - ваш руководитель,

По факту так. На самом деле в отделе 2 толковых программиста, но для ПК. Начальник, тоже вроде хорошо программирует, или кажется, что хорошо, но он как-то в железо не вникает. Я в отделе один, кто может быт и некрасиво программирует, но лучше всех знает железо (кортексы, ацп, шины и т.п.). Тут, без гордныни. Так и есть. Ибо все ко мне идут с вопросами, почему хардфолт, почему i2c не передаёт данные, почему spi не работает и т.п. Вот я и думаю, какого лешего меня грузить второстепенными вопросвми?

4 minutes ago, Corvus said:

1. Смириться

Пока этот вариант. Но с капелькой гнева.

5 minutes ago, Corvus said:

А это критично для больших долгоиграющих проектов.

Ну в моём случае придирки странные: ты завёл лишний класс, ты завёл целый файл ради 20 строк работы с таймером... А проект в стадии развития, и не учитывается, что сегодня этот класс работает только с таймером, а завтра туда может добавиться ПДП... Тем более, что самое интересное, нет отсылок к стандартам, которые это запрещают делать. В т.ч. и к внутреннему стандарту. Чаще отсылки к знаменитому Майерсу..

7 minutes ago, Corvus said:

Самодурство начальника не имеет к ревью никакого отношения.

ИМХО, начальник вообще редко должен вмешиваться в работу специалистов, уровень которых подтверждён зарплатой...

Share this post


Link to post
Share on other sites
2 minutes ago, haker_fox said:

Не стартап. Фирме уже более 25 лет. И давно и даже успешно выпускаем продукцию. Но эти придирки к коду... за...ют уже...

Тогда, возможно, вы чего-то не видите. В устоявшихся компаниях, как правило, не бегут за самыми новыми технологиями. Т.е. не увидишь сейчас в крупных устоявшихся компаниях C++20, даже C++17, 14 уже всё активнее появляется, но пока больше всего 11 или вообще 98. Это имеет минусы - нечем привлекать молодых и активных, но есть очень много кода, который написан уже в тех версиях языка, с идиомами и технологиями тех языков, со стилем, разработанным под них.

Упор начальника на стиль - это вещь правильная и объяснимая. Удивительно, при этом, что это он туда тащит новые фунции, а не работники. Что-то у вас не так, если работникам некогда интересоваться новинками. Обычно начильник, наоборот, является адвокатом старого стиля, и в той табличке, которую я вам советовал, в столбце с минусами у каждой новой технологии сильнейшим грузом лежит: "У нас гигабайты кода, написанные без этого. Какие проблемы это нововведение решает?".
И вот это тоже проблема, которую решает стиль кодирования. Есть вещи, над которыми думать нужно, а есть те, над которыми не нужно. И вопрос - где ставить фигурные скобки, где использовать передачу по ссылке, а где передачу по значению, как возвращать ошибки - через возвращаемое значение или бросать исключения - это то, что диктует стиль, и то, о чём думать не надо, то, что у всех в компании должно быть одинаково.

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

Share this post


Link to post
Share on other sites
7 минут назад, one_eight_seven сказал:

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

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

10 минут назад, one_eight_seven сказал:

Обычно начильник, наоборот, является адвокатом старого стиля, и в той табличке, которую я вам советовал, в столбце с минусами у каждой новой технологии сильнейшим грузом лежит: "У нас гигабайты кода, написанные без этого. Какие проблемы это нововведение решает?".

+1

Share this post


Link to post
Share on other sites
14 minutes ago, one_eight_seven said:

не бегут за самыми новыми технологиями.

У нас не совсем так... И представляете, как иногда трудно железячнику? Тебе и шины, и АЦП, и ЦАП и архитектуру МК и ещё кучу вещей нужно изучать, освежать в памяти. А тут ещё надо, оказывается, constexpr вместо const применять..

14 minutes ago, one_eight_seven said:

но есть очень много кода, который написан уже в тех версиях языка,

А это есть. Но всё равно для новых проектов пишется код с новыми реалиями...

14 minutes ago, one_eight_seven said:

Что-то у вас не так, если работникам некогда интересоваться новинками.

Смотря каким новинками. Я вот недавно занимался тем, что читал про архитектуру ARMv6-V, с которой предстояло работать после долгой работы с более старшими армами. Изучал ассемблер, в частности, чтобы более оптимально писать программы, если понадобится. Мне не интересно гнаться за новинками чисто языка. Более того, настораживает непонимание начальника ограничений архитектуры. Несколько лет назад по дурости я затащил std::map, std::string  и new в проект на STM32F051 с 8 кБ ОЗУ, поддавшись на соблазн того, что предоставляют эти штуковины. В итоге, всё было выкинуто где возможно, и заменено на более легковесные вещи. А кто мне это подсказал? О чём это говорит? О том, что человек видит верхушку, но не видит низов.

14 minutes ago, one_eight_seven said:

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

А такого у нас в регламенте нет...

14 minutes ago, one_eight_seven said:

Так что, корпоративный стиль кодирования - это вещь великая.

С этим согласен. Но в стиле всего, что начальник требует нет. Ещё никто стиль не поправил. Более того, вчера в ревью он указал коллеге, что не нужно инициализировать нулём стековые переменные типа size_t. Типа они автоматом инициализируются нулём. Это уже как-то совсем.

5 minutes ago, mantech said:

+1

а тож!

Share this post


Link to post
Share on other sites
1 минуту назад, haker_fox сказал:

что не нужно инициализировать нулём стековые переменные типа size_t. Типа они автоматом инициализируются нулём.

Из вас ему еще никто не предложил сесть и написать все самому? :biggrin: Эдак он и алгоритмы там "свои" модные предлагать начнет...

Share this post


Link to post
Share on other sites
1 minute ago, mantech said:

Вот никогда этого не понимал, зачем применять что-то другое (новое, модное и т.п.) если на том, что сейчас используется можно сделать то же самое? Ну вот зачем?

Затем, что часто это, действительно, удобнее, понятнее, лаконичнее.

Пример. Передача (присваивание) по позиции и по имени. Сначала появилось присваивание по позиции, и как только появилось и стабилизировалось присваивание по имени, я стал использовать её, и мой код стал гораздо понятнее, и всем это советую. Если вы знакомы с verilog, SystemVerilog, думаю, вам это известно - передача в модули уже давно подавляющим большинством инженеров используется только по имени. То же самое мождно видеть со структурами в C, C++  там всё чаще используется присваивание по имени поля.

Share this post


Link to post
Share on other sites
Just now, mantech said:

Из вас ему еще никто не предложил сесть и написать все самому?

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

1 minute ago, mantech said:

Эдак он и алгоритмы там "свои" модные предлагать начнет...

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

Share this post


Link to post
Share on other sites
5 minutes ago, haker_fox said:

ИМХО, начальник вообще редко должен вмешиваться в работу специалистов, уровень которых подтверждён зарплатой...

Тогда это не начальник, а заказчик. Или внутренний заказчик (вроде продакт менеджера, продакт оунера и т.д.)

А то получается, что тренер не должен вмешиваться в игру футболистов. Абсурд какой-то. :acute:

Share this post


Link to post
Share on other sites
1 minute ago, one_eight_seven said:

и всем это советую.

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

Share this post


Link to post
Share on other sites
Guest
This topic is now closed to further replies.