Как стать автором
Обновить

Комментарии 76

НЛО прилетело и опубликовало эту надпись здесь
На КДПВ всё наоборот. Заставлять человека читать кучу воды вместо того, чтобы просто указать ошибку – неуважение (исключение – когда ты сам влез к этому человеку с информацией об ошибке, никто тебя не просил о code review).

Воды много, но лучше написать "гайдлайн X не выполняется, перепроверить", чем тыкать в каждое место отдельно. Хотя бы во избежание "мне написали поправить А, Б и В — их я и поправил, а Г и Д тупой злобный ревьюер не заметил".

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

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

Вообще всё зависит от ревьювера, кому-то не лень откомментить каждый пробел, кому-то лень.

Другое дело это личные проблемы восприятия — надо быть закомплексованным человеком, чтобы воспрнимать коммент под каждым пробелом как давление на своё ЧСВ.
Вообще воспринимать всё в рамках ЧСВ и токсчиности — вредно. Ты не знаешь с кем этот человек общался до этого, и может ему уже надоело весь день отвечать на глупые вопросы а хочется уже поработать.

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

Я как-то предпочитаю "явное лучше неявного" — т.е. лучше сказать, что есть нарушения каких-то практик, в частности, в точках А, Б и В, но я не гарантирую, что это исчерпывающий список.


Вообще воспринимать всё в рамках ЧСВ и токсчиности — вредно.

Да ладно, учитывая пол и национальность автора оригинала — у неё ещё до фига взвешенная точка зрения.


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

Мега-труЪ. Особенно про "не бояться задавать вопросы".

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

Тут как раз просто. В первый раз даём общее замечание, если пришло второй раз — ставим метки везде, где эта ошибка есть, пришло в третий — издеваемся «бип»-ами. Если ничего не помогло, идём к руководству с вопросом о несответствии автора занимаемой должности.
Такие вещи, как на КДПВ вообще по-хорошему должны проверяться линтером на пре-коммит хуке и не давать класть такое в репозиторий.
Категорически согласен по поводу примера про «бип» — встречал подобное, хтелось просто убивать за такие «бипы». Ощущение самоутвеждения за мой счёт. Скажи один раз — мол, дружище, у тебя есть систематическая проблема, пробегись по коду. Не-е-ет, надо каждый раз носом натыкать, ЧСВ своё почесать!.. Извините, наболело)
А вот по поводу того, что вопрос «Почему вы не использовали ___?» некорректен — ак же категорически не согласен. Этот вопрос означает, что ревьюер НЕ считает себя всезнающим, признаёт, что его мнение не бсолютно, и интересуется чужим. Может быть, он просто чего-то не понял, и применять его любимый ___ здесь просто нельзя?
А вот по поводу того, что вопрос «Почему вы не использовали ___?» некорректен — ак же категорически не согласен. Этот вопрос означает, что ревьюер НЕ считает себя всезнающим, признаёт, что его мнение не бсолютно, и интересуется чужим. Может быть, он просто чего-то не понял, и применять его любимый ___ здесь просто нельзя?


В момем опыте чаще были примеры, когда ревьюер задает такой вопрос по причинам, которые не относятся к коду. Например если других вопросов нет, но хочется докопаться, чтобы человек не подумал, что у него все хорошо, а тут целый простор для дискуссии и морального доминирования.
Если человеку интересно чужое мнение, то он может его и в личной беседе спросить, не засоряя ПР.
Здесь вопрос может подразумевать, что программист знает и обязан (?) был сделать так.

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

Интервьюер ожидает не того, что программист только ответит на вопрос, а перенесет код в отдельный файл.

Дурость.
Спросили — ответил. Сказали сделать — сделал.
Не смешивать.
Статья большей частью выглядит как «Передача мнения как факта» )
Далеко не всегда надо все это соблюдать. Много частных случаев.
В моей практике были люди которые просто не понимали нормального отношения.
Им бесполезно было говорить «у тебя системная проблема».
Поэтому им и бикали и пикали и чего только не делали. А уволить их было невозможно…

Ваше мнение о статье не совпадает с моим.


Во-первых, любая статья — это всегда мнение. За фактами — это к криминалистами, да и там не сто процентов.


Во-вторых автор нигде не говорит о том, что его мнение — единственно верное. Более того, цитирую:


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

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


P.s. Не являюсь экспертом. Не претендую на истину в последней инстанции. Вы имеете право на мнение, отличное от моего.

Не просите разработчика исправлять проблемы «пока они здесь». Если код делает что требуется в тикете и не вводит новых проблем в кодовую базу, одобрите его, а затем создайте билет для очистки кодовой базы.

Плохая идея. Такие todo откладываются в долгий ящик в 99% процентах случаев, а призывы "ну давайте уже почистим наш код" отвергаются как начальством ("у вас это в спринт не входит, сделаете эту таску потом, когда завершите ещё n спринтов с мега-срочными фичами"), так и другими программистами ("у меня и так в жире куча незакрытых тасок висит, у меня нет времени").

Почему бы подразумевать по умолчанию исправление некоторого техдолга/рефакторинг в стандартный план? Точно так же как и тесты, кодревью и все остальное?
Я понимаю, что не везде это возможно по тем или иным причинам, но на мой взгляд стремится к этому уже будет хорошо. Хотя бы для себя

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

НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь

Например, меняет человек 1 строчку в инициализации класса — новый параметр константный.
А параметров там штук 5 и инициализируется он в 5 методах по всему файлу (и везде константы на месте объявлены). Я прошу исправить это, по факту ему придется весь файл переделать и + тесты.

НЛО прилетело и опубликовало эту надпись здесь
Предложение замечательное, окромя одного — в реальности (чаще всего) никаких Руководителей на ревью не напасешься. У нас есть тех дир, его конечно можно добавлять к ревью, но в реале он хорошо если в 1% случаев его просмотрит (просто тупо нет времени). Что там руководителей, у меня вообще просто в принципе ревьюера найти сейчас проблема (нужно править код по другой технологии, а ребята из другого отдела забиты работой по самое не могу, в итоге задержки по 1-2 недели на этом).
Я не встречал еще фирмы где есть такой избыток времени руководителей, если у вас оно есть — я безмерно рад за вас)
Если вы объединяете код, не ответив на все замечания, то это вызывает удивление у человека

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

Избегайте задавать осуждающие вопросы вроде такого:

Это не осуждающий вопрос. Если мне правда интересно, почему сделано А вместо Б, мне что делать?

Когда мне задают вопрос "почему вы не использовали ___?", у меня возникает только одна ассоциация — лошадь в ванне с огурцами.


Потому что я просто не вспомнил о ? Не знал о ? Не знаю почему не использовал ___?


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

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

Таким образом, если исполнитель не знал или что-то забыл, то теперь он узнал/вспомнил, а если у него есть аргументированное объяснение, то, возможно, что-то полезное узнаю я.
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь
Gerrit так позволяет.
НЛО прилетело и опубликовало эту надпись здесь

Имхо все проблемы решает вежливость. Достаточно написать "Please change X" или "Please consider X instead" и на этом все вопросы токсичности можно закрывать

По КДПВ: есть коллеги, с которыми второй вариант не работает, они реагируют только на прямые указания, потому как в ревью борде это дефекты и их нужно закрыть. А просто просмотреть самостоятельно, хотя бы тот же файл, на предмет аналогичных ошибок… никак.

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

Меня вот это зацепило. Если в коде происходит сбой, то о каком «если не сложно» может идти речь? Это должно быть исправлено.
НЛО прилетело и опубликовало эту надпись здесь
Да, но можно же сказать: «Поправь этот баг, пожалуйста». Я к тому, что постановка фразы подразумевает, будто у человека есть возможность сказать «нет». Зачем это все, если мы оба знаем, что баг придется поправить вне зависимости от того, сложно человеку или нет?
НЛО прилетело и опубликовало эту надпись здесь

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


Поэтому не могли бы вы вытащить этот нож из моего черепа, пожалуйста? Это доставляет мне дискомфорт, т.к. кровь заливает мне глаза. Как у вас будет свободное время конечно же.

Это уже особенности перевода и особенности их речи.

В оригинале
This breaks when you enter a negative number. Can you please address this case?


В нашем переводе как раз и получается «Исправьте ошибку, пожалуйста». Кстати автор перевода этот момент с «пожалуйста» уже исправил.

То, что использовано «Can you please address this case?» вместо прямого «Address this case.», то это как уже упомянули, особенность их речи, не принято в повелительном наклонении выражаться.
Все понятно, спасибо за разъяснение. В оригинале действительно нет этого «если не сложно», которое меня и зацепило. К отсутствию повелительного наклонения у меня претензий не было, в английском оно действительно предпочтительней.

ИМХО, есть одно простое правило на все случаи жизни:
"Поступай с другими так, как хочешь, чтобы поступали с тобой".
Все токсичные практики (и не только в it) — всегда пример обратного.

Это правило не так-то хорошо работает, когда на одном конце провода незакомплексованный человек, а на другом конце провода — человек, который находится в состоянии стресса (или просто не имел конструктивного опыта взаимодействия с критикой).
Много проблем происходит из-за того, что люди разные и не понимают, что их поведение вызывает негативные ощущения у других.
Ну, я вот считаю, что, якобы, «токсичный» вариант из КДПВ, это коротко и по делу, а «вежливый» — это бессмысленная трата времени обоих сотрудников.
И, получив подобное ревью, даже не подумал бы, что оно, оказывается, «токсичное».

image


И хоть бы слово о том, что не надо вообще обращать внимание на такие мелочи.

НЛО прилетело и опубликовало эту надпись здесь
поправит в этом файле одну строку, прогонит файл через стандартный форматировщик

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

НЛО прилетело и опубликовало эту надпись здесь

Я вполне однозначно процитировал за что.

НЛО прилетело и опубликовало эту надпись здесь

С их комбинацией.

НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь

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

Если я правильно вас понял, то вы предлагаете


  1. Oставить trailing spaces без внимания, т.к. это мелочь которую не надо замечать. Пусть она и нарушает code style
  2. Запретить этому и любому другому разработчику исправлять пробелы в PR с полезной нагрузкой. И бить по рукам тем, кто так сделает.
  3. Приводить оформление к оговоренному стилю только когда в файле ужас-ужас, но не раньше, когда просто ужас.

Я правильно вас понял?


Я с таким подходом никогда не сталкивался, и (теоретически) это провоцирует к отказу разработчиков применения авто-форматирования в IDE, ведь из-за автоформата придется проверять не сбились ли пробелы с "как было" на "как надо". Это, в свою очередь, приведет к постепенному нарастанию кома кода вне оговоренного стиля.
Если у вас есть опыт работы команды с таким подходом, поделитесь пожалуйста, насколько эти проблемы реальны? И какого уровня разработчики были в команде?

Какой ужас. Код с лишними пробелами ведь просто невозможно читать. Развели тут карго-культ на тему код-стайла.


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

Дело не в пробелах. Это малая часть code style. И дело не в карго-культе. Из ваших ответов я делаю вывод, что вы не считаете важным причины, по которым возникло понятие единого стиля кодирования для команды.
Возможно, вам не встречались коллеги, которые пишут цикл в одну строку, else if разносят на 3 строки и ленятся настроить форматтер в IDE, и тем более его применять. Возможно, вы не понимаете, насколько замедляют чтение и понимание такие нестандартные куски кода.
И вероятно, вы не получали по шапке от тимлида за смешивание в одном PR новый код и форматирование, когда приводите такие куски к удобочитаемому формату. Вы из тех, кто ругает за улучшение общей кодовой базы.

Возможно, вам не встречались коллеги, которые

Вот, вы сами пишите, что проблема в коллегах, а не "единстве стиля". Если коллега пишет говнокод, то надо либо объяснить ему что не так (или он объяснит вам что всё так), либо перестать с ним работать, если он не понимает разумных тезисов.


вы не получали по шапке от тимлида

Я никогда не даю по шапке за такие глупости. Го к нам работать.)

Я все меньше понимаю вас.


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

совсем не вяжется с


Я никогда не даю по шапке за такие глупости

Не бьете, потому что не дают? Использовали сильную фигуру речи, хотя так не считаете?


Это должны быть разные пулреквесты. Причём второй нужен лишь в совсем запущенных случаях типа "все стили в одну строку"

Так все-таки это глупости, на которые вы не обращаете внимания, или вещи, достойные отдельного пулл-реквеста?

Для изменения одной строчки не надо изменять весь файл, какой бы благородный мотив ни был. Исключение — легаси-говнокод — от него надо избавляться как можно быстрее.


А вот эту строчку форматировать можно по разному. И как именно — не особо принципиально в разумных пределах.

НЛО прилетело и опубликовало эту надпись здесь

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

Вот разные PR — это как раз вредный совет. Одновременно-то их не сделать...

Почему же не сделать?

НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь

Статья оставляет двойственное ощущение. С одной стороны, написаны правильные и интересные вещи. С другой, я вижу некие внутренние противоречия, и это снижает доверие к статье в целом:


  1. С одной стороны, "стиль и синтаксис не имеют ничего общего с проблемой, которую изначально решает разработчик". С другой, обсуждается комментарий с пробелами — что является следование стилю. Так соблюдение стиля в коде PR — это допустимо или нет?
  2. Несколько раз озвучивается тезис, что главный критерий — решает ли PR задачу, поставленную перед исполнителем. С другой стороны, "Полезное поведение № 5: Используйте возможности для обучения" говорит, что даже если задача решена, стоит комментировать для обучения другого человека. Когда он этого не просил. Так, чтобы не вызывать у него негатива. Я не говорю, что так сделать невозможно. Но все-таки, автор, в чем цель код-ревью?
  3. Может, я делаю неправильные выводы из "Полезное поведение № 10: установите стандарт, пока команда мала и растёт". Но прям слышу продолжение "… и не поощряйте изменения стандарта, когда команда выросла". Что вы будете делать, если проекту 3-5-10 лет, а актуальные стандарты уже другие?
    • Писать новый код по новым стандартам, старый оставлять и мучиться с двоестандартием?
    • Убеждать собственников потратить неопределенное количество сил на переписывание старого работающего по новому?
    • Требовать писать новый код по-старому, и решать проблемы с недовольством команды?
Ощущения от прочтения: чем дальше в индустрию, тем больше правят массовики-затейники, а не инженеры. Общий смысл в том, что типа не дай бог нарушить зону комфорта нашего васеньки… Не вздумай ничего сказать прямо, пусть он сам догадается, а жёлтую карточку покажет система, человек не может, потому что мы же ж одна команда, зачем всё портить…
«Будьте осторожны, чтобы не выказать удивление, если человек чего-то не знает.»
— я однажды натурально язык от удивления проглотил, когда узнал, что коллега js-ник после трёх лет работы на работе не знал конструкции const self = this. Как показала дальнейшая практика взаимодействия, пинка ему надо было выдавать как можно скорее. К сожалению, тогда в компании вот так же носились с сотрудниками в духе «не надо, просто подождём, люди ведь хорошие и учатся», что в итоге просто закончилось бесконечным исправлением косяков и переработками тех людей, которые умеют работать и вынуждены убирать говно за такими васеньками…

По-моему, достаточно просто базовой рабочей этики, вроде избегания конфликтов, стремления быть конструктивным и так далее. Прирождённых интриганов все эти ваши правила не остановят: они найдут способ ущипнуть. Зато иногда драматически удлиняется путь коммуникации между сотрудниками, люди думают про тонны словесного поноса, который они вынуждены писать, чтобы донести простую мысль.
Добавлю свои пять копеек. То что встречается в работе, то что вспомнилось.
  • слишком долгий ответ на pull request. Что имеется в виду: один из участников ревью слишком долго не реагирует, не ставит approve и не пишет замечаний. Но код из этого пулл река кому-то срочно нужен. В итоге либо получается простой в работе, либо утверждение запроса происходит не дожидаясь кого-то из участников.
  • Легкомысленное принятие запроса. То есть ставим approve не просмотрев код
  • Игнорирование замечаний. Автор, не смотря на наличие замечаний сам завершает pull request. Или просто сам создает, сам апрувит, не дожидаясь никого

PS: понимаю что часть проблем решается настройкой политики проведения PR
Код-ревью еще та гадость. Тот кто это придумал и поддерживает нуднейший человек. Устроили «бюрократию» их хобби программировать, то только отвращение от работы вызывает. Программирование само по себе испепеляющая и нудная вещь, так её еще и корпоративным адом испортили. Причем ревью делают с целью самоутвердиться, так как у кодеров больше всего комплексов, которые сублимируют из всех профессий. И выходит так якровыраженный синдром вахтера, особенно у тим-лидов. Хуже только у рекламщиков и дизайнеров делающих рекламу, там поголовно у всем чувство величия и комплекс бога. Это еще принято запивать алкоголем и заедать жирным от осознания собственной несчастности.
Зарегистрируйтесь на Хабре , чтобы оставить комментарий

Публикации

Истории