Comments 49
Я обнаружил, что лучше всего задавать открытые вопросы — это не агрессивно и даже побуждает разработчика мыслить критически. Это займет больше времени, чем просто рассказать кому-нибудь решение? Да, краткосрочно. Но в долгосрочной перспективе вы помогаете им расти, и они с меньшей вероятностью повторят свои ошибки.
Итак, в следующий раз, когда кто-то откроет файл внутри цикла for, а не перед ним, вместо того, чтобы просто указать на это, спросите: «Как бы вы уменьшили сложность здесь?». Это будет много значить.
Фидбак адекватный получить нереально.
Сдаешь выполненное ТЗ, вплоть до буквы, и начинается «что-то мне не нравится, как Вы думаете что можно было бы улучшить» и иди блин 3 дня размышляй в попытках угадать что заказчику не понравилось.
Вроде найдешь то, чем думаешь проблема, ускоришь загрузку сайта в 2 раза, а он такой разочарованно «ну вот, ничего не поменялось» и после еще 3 суток попыток выяснить в чем дело уходит жалоба выше вроде «плохой работник, уже неделю не может поменять цвет иконки»©
Ну ладно, это лирика. К конкретике.
Вопрос «как бы Вы уменьшили сложность тут» ужасен по 2 причинам.
1) Это вопрос в стиле «Вы перестали пить коньяк по утрам». Почему? Потому что ревьюер не зная причин такого решения уже заведомо решил что это решение неправильное, даже не интересуясь причинами.
2) Этот вопрос не дает даже примерно информации о проблеме о которой говорит ревьюер. Как правило, если кодер что-то сделал, он это сделал не потому что он саботирует проект, а потому считает что это правильно. Поэтому с хорошей степенью вероятности он не поймет о чем именно речь и в лучшем случае потеряет тучу времени в догадках, заодно убеждаясь в своей непроходимой тупости — что он не может сразу сходу найти проблему.
Правильно было бы спросить «по каким причинам Вы выбрали Вы открываете файл в цикле? не считаете ли Вы что это неадекватно увеличит сложность?».
В такой постановке во-первых, сразу идет разговор по делу и прогер не занимается гаданием; во-вторых, ревьюер оставляет за собой право на ошибку.
Это у всех так. Попробуйте внимательно понаблюдать за интонациями коллег при обсуждении каких-либо разногласий. Почти каждому нужно эмоционально самовыразиться.
Просто у нас это настолько повсеместно, что считается нормальным.
Так не правильно, потому-то
Это не сработает, так как
Это сложно/Зачем это
Найди другой способ решения согласующийся с
Человек испугался :(
Вообще не важно человек писал код 5 минут или час или скопировал где. Как только пишешь, что мол код негодный и вот места с вопросами — мотивация у него падает и ещё агресия встречается.
Люди сами не могут код от себя отделить.
Люди не могут просто так отказаться от своей идеи.
Люди не хотят тратить время на переделывание, «так как делают по максимуму», а это «больше придирки и вкусовщина» и даже жалуются на выгорание и отсутствие мотивации.
Часто получается так, что они хотят ПР пройти, а не код написать.
Мальчику стажеру я когда-то на пул реквестор в 20 строк написал много замечаний и рекомендаций прикрепив примеры и вышел за рамки экрана, а так же использовал такое:Мы бы благодарны были, разве что в ноги не поклонились бы. На курсах блин отваливаешь по косарю в месяц что бы препод хотя пару раз в неделю говорил что ты не так делаешь по паре пунктов, а тут подробнейший ревью по делу от практикующего специалиста и нос воротить от такого?
Так не правильно, потому-то
Это не сработает, так как
Это сложно/Зачем это
Найди другой способ решения согласующийся с
Человек испугался :(
Люди сами не могут код от себя отделить.И что они делают в программерах? В профессии, когда обучение никогда не заканчивается по определению?
Люди не могут просто так отказаться от своей идеи.
Мой опыт: Я с русскоговорящими в том числе работаю. И казалось бы такая «закалка» и «конструктивная» критика, которая даётся в СНГ должна давать преимущество.
Но я все время сталкиваюсь с такой проблемой, что люди не готовы озвучивать свои мысли.
«Я это сделаю. Я все понял» через неделю заканчивается тонной велосипедного кода.
А в начале же говоришь, что можно созвониться или початиться или мыло отправить, если есть вопросы или недопонимание. Но нет.
А помощь нужна? Конечно же нет.
У «толерантных» коллег меньше проблем и созвониться и прогресс показать, и закончить велосипедить, начав код писать.
Ну это по моим личным наблюдениям.
Поэтому с хорошей степенью вероятности он не поймет о чем именно речь и в лучшем случае потеряет тучу времени в догадках, заодно убеждаясь в своей непроходимой тупости — что он не может сразу сходу найти проблему.С другой стороны, сталкивался с такой проблемой — объясняешь человеку суть за 5 минут, он все понимает, но тут же забывает и в следующий раз опять спрашивает. Но если заставить его думать, то найдя решение через час, он запоминает его надолго.
Конечно подход не универсальный и не для всех задач, но те решения до которых додумался самостоятельно запоминаются намного прочней.
Я бы переформулировал эту фразу более нейтрально: «Этот файл открывается в цикле, это может сильно сказаться на сложности/производительности кода». Желательно дополнить это еще советом (но не указанием, автор может придумать что-то лучше чем вы) о том, как этот код можно улучшить. Например: «Попробуйте переписать код так, чтобы открыть файл один раз перед циклом и переиспользовать дескриптор»
Думаю все-таки очень важно следить за лексиконом во время код-ревью. Даже такие слова как «неадекватно» могут выглядеть чересчур резкими. Помните, что вы и без этого в позиции силы, аппрувал кода зависит от вас и нет нужды дополнительно демонстрировать эту силу.Вот это и есть проблема с чрезмерной толерантностью.
Во-первых, термин «неадекватно» (применительно к коду/решению) это всего лишь «не соответствующий чему-либо, неподходящий для чего-либо». Таким образом когда Вы называете это слово слишком резким — Вы ставите субъективное мнение выше объективного значения и тем самым открываете врата ада. Когда кодер должен гадать не только о том, где он сделал что-то не так, но и еще гадать как бы ему ответить так, что бы это не звучало для ревьюера слишком резко с точки зрения субъективного мнения ревьюера. Подобная атмосфера не способствует взаимопониманию и продуктивности.
Во-вторых, использование Вами фразы «с позиции силы» предполагает что в коллективе нет здоровых, партнерских отношений и взаимопомощи, а есть отношения подавления и силы и hostile environment (в голову не приходит как сказать это по русски, извините). И проблема именно в этом, использование терминологии может маскировать эту проблему, но никак ее не решит. В коллективе должна быть такая атмосфера, что бы наличие позиции силы у вышестоящего не подразумевалось, тогда любые замечания будут восприниматься в позитивном ключе.
Я бы переформулировал эту фразу более нейтрально: «Этот файл открывается в цикле, это может сильно сказаться на сложности/производительности кода».Как бы вопрос не формулировался — первое что должен сделать ревьюер — узнать почему код написан так, как он написан. Выяснив причину — может оказаться что код на самом деле написан верно. И даже если не верно — выяснение причин поможет избежать аналогичных ошибок в будущем, а не просто поможет устранить эту конкретную.
Подобная атмосфера не способствует взаимопониманию и продуктивности.
Полностью с вами согласен, субъективность восприятия людей все меняет. Возможно мне не стоило поднимать эту бессмысленную дискуссию о степени резкости одного слова, но мой глаз за него зацепился в изначальном комментарии.
есть отношения подавления и силы и hostile environment
Быть в позиции силы не означает, что ее нужно использовать, но все же у ревьюера она есть, и это факт, о котором стоит помнить.
тогда любые замечания будут восприниматься в позитивном ключе
Не могу согласиться со словом «любые», опять-таки это слишком субъективно и нужно помнить что разные люди воспринимают жесткую критику по-разному.
P.S.
Вот это и есть проблема с чрезмерной толерантностью.
Чисто из интереса: считаете ли фразу, которые я предложил как коменты в код-ревью излишне толерантными? Такой же вопрос к общему стилю моих комментариев в этом треде.
Быть в позиции силы не означает, что ее нужно использовать, но все же у ревьюера она есть, и это факт, о котором стоит помнить.Наличие у ревьюера позиции силы, имхо, означает неправильную огранизацию воркфлоу. Достаточно кодеру дать прав апелляции и позиция силы либо нейтрализуется, либо оказывается у кодера (в случае если ревьюеру прилетает за его ошибки сильнее чем кодеру).
Возможность «пожаловаться» должна быть абсолютно штатной и должна уравновешивать «силы». И это даже вопрос не толератности, а продуктивности.
Не могу согласиться со словом «любые», опять-таки это слишком субъективно и нужно помнить что разные люди воспринимают жесткую критику по-разному.Воспринимают по разному. Но если кто-то воспринимает замечания к работе слишком остро, то проблема на его стороне. Именно потому, что это слишком субъективно и именно воспринимиющий отвечает за свое восприятие.
Чисто из интереса: считаете ли фразу, которые я предложил как коменты в код-ревью излишне толерантными? Такой же вопрос к общему стилю моих комментариев в этом треде.Излишне толерантными — нет, вот вообще ни разу, если отвечать строго на этот вопрос.
Будьте осторожны, чтобы не переусердствовать, хотя.
Ну нельзя же настолько откровенный гуглоперевод публиковать!
Тут проблема в том, что этот совет неконструктивен. Чтобы его выполнить, нужно определить, что такое эта «одна вещь». Собственно, вы это почти и иллюстрировали своим комментом.
Кроме того, разбить PR на несколько мелких — значит получить в N раз больше PR, которые непонятно как связаны друг с другом (потому что проследить эту связь между разными PR весьма проблематично). Такое лечение вполне может оказаться хуже болезни.
>Добавьте флаг «Я запускал этот код локально»
Во многих моих проектах код просто локально не запускается. Например потому, что работает в среде Hadoop — а локальные имитации кластера это просто убожество. Я понимаю, почему эту идею поддерживают, но она далеко не универсальна.
Почти все идеи неуниверсальны. Это не значит, что их не надо поддерживать. Надо просто применять там, где это пододит.
Наверное здесь "локально" означает "в тестовой среде" она может быть нелокально, а в облаке, просто код в тестовом инстансе развернут из данного конкретного бранча. Судя по тексту, автор имел ввиду "я провел ручное End-To-End тестирование этого кода"
В общем читать здесь: www.atlassian.com/git/tutorials/comparing-workflows/forking-workflow
Ну ок, один человек ведет свою работу в своем репозитории. И делает несколько задач.
Понятно что у него-то все будет хорошо — пока не нужно будет эти таски объединять с чужими в рамках одного релиза. Но в этот-то момент что помешает возникновению конфликтов, например?
Автор всегда имеет удалённую копию и может синхронизировать свою работу.
Всегда можно посмотреть кто над чем работает.
Работа разработчика не блокируется, если пул реквест отсрочен. Он может продолжать мерджить из мастера и продолжать свою работу.
И такси надо объединять не в рамках релиза, а постоянно.
Вы имеете ввиду что на одного разработчика ровно одна ветка? Или что ветка на фичу И разработчика?
Ссылка на forking workflow она про создание форков репы а не про ветки.
Если разработчик заводит ветку на инкремент (багу или новую фичу) то все плюсы, которые у вас упомянуты уже соблюдаются.
Разрабочик создает ветку, ассоциируюет ее с таксой в багтрекере — сразу видно кто занимается, после того как он закончил работу он делает пулл реквест и изменение попадает в мастер после ревью. В процессе работы он может мерджить свежий мастер не блокируется другими людьми.
Во первых возникает туева хуча веток. Часто работу нельзя правильно разбить по этим веткам, да и не нужно.
Если ревьювер занят, то накапливается куча веток которых надо смержить друг в друга а также в локальную ветку. Это крайне неудобно.
Кроме того разработчики часто работают над несколькими задачами одновременно. Надо постоянно переключаться между ветками.
Непонятно куда девать коммиты которые не связаны непосредственно с тасками, либо таски не созданы, либо уже закрыты.
Forking workflow же вообще не имеет никаких недостатков и выглядит более элегантно.
Во первых возникает туева хуча веток. Часто работу нельзя правильно разбить по этим веткам, да и не нужно.
Что такое "правильно разбивать" и почему их невозможно разбить?
Кроме того разработчики часто работают над несколькими задачами одновременно. Надо постоянно переключаться между ветками.
Что их заставляет так делать? Как они поступают когда одна задача готова, а другая — еще нет? Как потом в истории разобраться зачем именно было сделано конкретное изменение?
Непонятно куда девать коммиты которые не связаны непосредственно с тасками, либо таски не созданы, либо уже закрыты.
Давайте, переведу на русский — непонятно, куда девать комиты которые не связаны с задачами, либо задач нет либо они уже выполнены.
С моей точки зрения, все коммиты связаны с задачами, если у вас нет никакой задачи не создавайте коммит. Если у вас есть задача, оформите это в багтрекере. Если у вас коммит связан с закрытой задачей, значит в багтрекере вранье — либо задача не закрыта, либо это новая задача, связанная с данной.
Что их заставляет так делать? Как они поступают когда одна задача готова, а другая — еще нет? Как потом в истории разобраться зачем именно было сделано конкретное изменение?
Что значит зачем? Программирование — это творческий процесс. Разработчики работают, как им удобнее. Если у меня есть 2 задачи по фронту и бакенду для фичи A и фронт+бакэнд для фичи B.
То мне может показаться удобнее написать сначала бакэнд для обеих фич а затем фронт для обеиз фич.
Как потом в истории разобраться зачем именно было сделано конкретное изменение?
В коммитах должно быть описание. Ваши ветки в этом, кстати, мало помогают. Ибо разработчики смотрят историю основной ветки (как правило).
С моей точки зрения, все коммиты связаны с задачами, если у вас нет никакой задачи не создавайте коммит. Если у вас есть задача, оформите это в багтрекере. Если у вас коммит связан с закрытой задачей, значит в багтрекере вранье — либо задача не закрыта, либо это новая задача, связанная с данной.
То что вы описываете-это самая настоящая бюрократия. И она плохо ложится на реалии.
Я например постоянно переписываю документацию. И что мне для каждой опечатки таску и отдельную ветку создавать?
Что значит зачем? Программирование — это творческий процесс. Разработчики работают, как им удобнее. Если у меня есть 2 задачи по фронту и бакенду для фичи A и фронт+бакэнд для фичи B.
То мне может показаться удобнее написать сначала бакэнд для обеих фич а затем фронт для обеиз фич.
Ок тут согласен — вполне допустимо в каких-то случаях создавать ветку на несколько связанных задач, если их удобнее сделать как одну.
Но вы-то предлагает ветку на разработчика — т.е. там должны быть вообще все зачачи разработчика даже логически не связанные или как?
В коммитах должно быть описание. Ваши ветки в этом, кстати, мало помогают. Ибо разработчики смотрят историю основной ветки (как правило).
Наверное тут какое-то недопонимание. Я подразумеваю, что ветки сливаются в какой-то момент в основную. Соответсвтенно, merge commit будет содержать описание и ссылку на задачу и можно будет посмотреть в истории по master.
История в фичебранчах как правило не нужна другим разработчикам, кроме того, случая, когда они совместно работают над долгоживущим бранчем. Но этот случай вообще непонятно, как решать в режиме "ветка на разработчика".
Я например постоянно переписываю документацию. И что мне для каждой опечатки таску и отдельную ветку создавать?
Ага, с моей точки зрения это не должно занимать сильно больше времени, чем написание данного коммента.
Но вы-то предлагает ветку на разработчика — т.е. там должны быть вообще все зачачи разработчика даже логически не связанные или как?
Совершенно верно. Собственно так это и задумывалось: youtu.be/4XpnKHJAok8?t=1599
Я подразумеваю, что ветки сливаются в какой-то момент в основную. Соответсвтенно, merge commit будет содержать описание и ссылку на задачу и можно будет посмотреть в истории по master.
Merge commit будет содержать название ветки, но что мне это даст? Не проще ли просто посмотреть в комментарии к коммитам.
История в фичебранчах как правило не нужна другим разработчикам, кроме того, случая, когда они совместно работают над долгоживущим бранчем. Но этот случай вообще непонятно, как решать в режиме «ветка на разработчика».
Как фича бренчи улучшают понимание, если разработчики синхронизируются только с мастер веткой? Они могут появляться и исчезать без всяких следов. Так зачем их создавать тогда?
Я не спорю что есть ситуации, когда надо действительно сделать какую-то отдельную изолированную ветку длиной в полгода. Но зачем засорять гит тысячами мелких бренчей мне непонятно.
Ага, с моей точки зрения это не должно занимать сильно больше времени, чем написание данного коммента.
Это сильно раздражает. Это всё равно что раз в час писать в журнал что сделал за последний час — недолго, но мерзко
Совершенно верно. Собственно так это и задумывалось
Приведите цитату плиз, я там услышал что им удобно репо на команду, а не ветку на разраба
Merge commit будет содержать название ветки, но что мне это даст?
Обычно он содержит ссылку на PR которую можно открыть и посмотреть все детали
Как фича бренчи улучшают понимание, если разработчики синхронизируются только с мастер веткой?
Если они синхронизируются с мастером, то, по крайней мере, можно для работе в процессе понять какие изменения для какой фичи предназначены. Можно так же мерджить их в мастер пофично (всегда решить какую фичу вмерджить, а какую — нет). Для готовой работы есть уже мастер с его историей.
Но зачем засорять гит тысячами мелких бренчей мне непонятно.
Так они его не засоряют. Обычно они живут пока фича разрабатывается и умирают после мерджа в мастер — причем, это автоматически происходит при завершении пулл реквеста. При следовании соглашениям о наименовании для бранчей типа feature/AddLoginDialog их можно не видеть, пока явно не захочешь, но зато, если захочешь можно увидеть.
Обычно он содержит ссылку на PR которую можно открыть и посмотреть все детали
Почему нельзя взглянуть сразу в коммит сообщения, где так же будет ссылка на таску в джире (если коммит связан с таской). Зачем мне идти в мердж коммит, который фиг знает где, открывать пул реквест и только там смотреть конкретные таски?
Можно так же мерджить их в мастер пофично (всегда решить какую фичу вмерджить, а какую — нет). Для готовой работы есть уже мастер с его историей.
Зачем что-то мержить в местер пофично? Если какая-то фича не вмержена в мастер, то я как разработчик фичи нахожусь в подвешенном состоянии ибо не знаю, какую версию кода брать для дальнейшей работы над другими задачами: с фичей или без неё.
Почему нельзя взглянуть сразу в коммит сообщения, где так же будет ссылка на таску в джире (если коммит связан с таской). Зачем мне идти в мердж коммит, который фиг знает где, открывать пул реквест и только там смотреть конкретные таски?
Что значит фиг знает где? Он составляет историю мастера.
Я совершенно не против, чтобы там были и идентификаторы фич. Просто если их много удобнее видеть ссылку PR в целом и текстовый осмысленный коммент, если нужны детали можно открыть PR и ходить по ссылкам на фичи если их несколько.
Если она одна, наверное, иметь ссылку на PR чуть менее удобно, чем на фичу в джире на один дополнительный клик. Зато имея ссылку на PR можно посмотреть и дополнительные вещи (типа кто ревьюил и какие были замечания.)
У меня обычно наиболее гранулярные коммиты содержат атомарные правки типа "переименование метода", который мало кому интересны кроме меня, но я зато всегда могу откатиться на предыдущую версию в процессе работы над фичей, но там обычно нет ссылки на багу. На багу ссылается ветка в целом.
Зачем что-то мержить в мастер пофично?
Потому, что вы можете работать, как вы сами говорили, над несколькими фичами, одна может быть готова отправке в мастер, другая не готова — тесты не дописаны, не компилируется, нет документации или какие там у вас характеристики проверяет билд еще.
какую версию кода брать для дальнейшей работы над другими задачами: с фичей или без неё.
Если она не вмерджена в мастер, значит не готова. Берите всегда из мастера. (Кроме того случая, когда вы работаете над долгоиграющей фичей — тогда берите из фичи, над которой работаете)
Что значит фиг знает где? Он составляет историю мастера.
Вы пытаетесь использовать ветки для документации истории изменений. Но они для этого не предназначены. Они нужны для изоляции кода.
Для документации изменений существуют комментарии к коммитам, где может быть любая исчерпывающая информация. Мердж коммит находится «вдалеке» от основного коммита и надо искать где находится тот, про который написано в мердже.
Потому, что вы можете работать, как вы сами говорили, над несколькими фичами, одна может быть готова отправке в мастер, другая не готова — тесты не дописаны, не компилируется, нет документации или какие там у вас характеристики проверяет билд еще.
А в мастер не обязательно мержить, когда вся фича готова. Можно замерджить и 10 фич и полфичи. Главное — чтобы существующий код не ломался и был проревьювен.
Если она не вмерджена в мастер, значит не готова. Берите всегда из мастера.
Необязательно: она может быть сделана, но непроревьювена.
Вы пытаетесь использовать ветки для документации истории изменений. Но они для этого не предназначены. Они нужны для изоляции кода.
Как раз для изоляции — фич друг от друга.
Для документации изменений существуют комментарии к коммитам, где может быть любая исчерпывающая информация.
Коммиты бывают разные — например в процессе разработки фичи могут быть эксперименты, откаты на предыдущие версии, мердж с мастером и так далее.
Все это нужно до того момента, когда надо интегрировать фичу, после того как фича готова, это можно засквешить.
Мердж коммит находится «вдалеке» от основного коммита и надо искать где находится тот, про который написано в мердже.
Делается squash merge и я вижу только один коммит. Если не хочется сквешить, то можно проребейзить перед мерджем и они будут рядом.
А в мастер не обязательно мержить, когда вся фича готова. Можно замерджить и 10 фич и полфичи.
Чем больше объем мерджа тем больше разбираться что сломало CI, меньше гранулярность при черрипике, если надо будет, больше объем ревью и так далее.
Полфичи можно, но тут надо понять, чтобы не было неконситентного нового наблюдаемого поведения. Если его нет, то либо эти пол фичи можно выделить в фичу, либо будет мертвый код. Хотя, это, в принципе допустимо, многие так делают изолировав новое поведение фичетаглами.
Необязательно: она может быть сделана, но непроревьювена.
Зачем ее тогда брать, после ревью может быть сильно изменена?
Я бы сказал, что это нетипичный случай.
Как раз для изоляции — фич друг от друга.
Ну правильно. Но зачем изолировать задачи выполняющиеся одним и тем же автором, на протяжении короткого времени и часто вперемешку. А после мерджа в мастер всё равно обе модели выглядят по сути одинаково.
Даже если допустить что мерж коммиты дают кое-какую дополнительную информацию, то на практике я не припомню, чтобы я когда-либо с этим заморачивался. Обычно я смотрю на конкретные коммиты.
Чем больше объем мерджа тем больше разбираться что сломало CI, меньше гранулярность при черрипике, если надо будет, больше объем ревью и так далее.
Ну вообще это больше зависит от пропускной способности ревьюверов, а не от бренчинг модели.
Вообще, конечно, чем чаще мерджинг тем лучше, хоть полфичи хоть четверть. Feature branch модель скорее неявно поощряет откладывать мерджинг, до тех пор пока таска не полностью завершена
Надо постоянно переключаться между ветками.
Непонятно куда девать коммиты которые не связаны непосредственно с тасками, либо таски не созданы, либо уже закрыты.
А в случае с одной веткой на разраба как эта проблема решается? Просто всё льём в одну ветку?
Да, кстати, 1000 ревью за три года — это всего лишь примерно одна штука в день. Я вообще не вижу, чем тут хвастаться, у меня постоянный поток код-ревью от коллег примерно такой. Автор уже три года как стал миддлом, и начал делать код ревью? Ах, какое достижение…
>Нельзя никогда задавать открытые вопросы на код-ревью.
Ну, в какой-то мере код-ревью — это все-таки средство для обучения менее опытных более опытными коллегами. А в обучении разные подходы могут быть хороши, смотря кто перед вами. Но лишь могут…
Поэтому мне кажется, тут вообще нет одного правильного ответа. И совет «всегда задавайте открытые вопросы», и совет обратный — они оба не вполне универсальные.
Добавьте флаг «Я запускал этот код локально»
как раз лишний пункт,
то что компилируtтся, запускается и работает — должно проверятся тестами в build pipeleine
Что я узнал после более чем 1000 code review