Pull to refresh

Comments 71

Почему минусуют ответ на комментарий, который в точности повторяет первый? Потому что нельзя повторяться?
Потому что похоже на "+1", а для этого есть специальная кнопка. А у кого не работает кнопка — мол, сам себе злобный буратино :)
один раз можно себе позволить насобирать минусов
Это звучит как: сапер ошибается один раз. :)
Одно из правил в Гугле запрещает аргументы по умолчанию (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments). Причина — часто при использовании таких функций люди просто копируют их из другого куска кода и заменяют параметры, не ознакамливаясь с объявлением самой функции и передавая параметры по умолчанию, которые могут в данном контексте быть неверными. Т. е. ситуация такова, что легче смириться с криворукостью кодеров и подогнать под них правила, чем выпрямить им руки.

Вам всё ещё хочется работать в Гугле?
либо же это позволит ему устранить один из этапов при сохранении собственного кода в репозиторий

Я считаю, что это не верно, т.к эксперт тоже может ошибиться. Ревью сделаны не для «лохов», а для того, чтобы минимизировать опечатки\ошибки любого члена команды, в том числе и «readability expert'а»
ошибки и опечатки проверяются совсем другими инструментами. потому они и называются readability, а не punctuation/syntax эксперты.
ОК, я расширю пример. Допустим, эксперт писал модуль, большой и дремучий, после написания всего функционала он провел рефакторинг, но вот после рефакторинга остался некоторый избыточный код. Эксперт его сходу не видит и забывает о нем. Кто найдет этот лишний код? Только не надо мне рассказывать про «экспертность» экспертов, все мы люди, всегда что-то да забываем ;)
А вот это принимаю. Как эксперт )))))))
ревью должно проходить не однажды после написания «дремучего модуля», а на промежуточных стадиях.

В этом случае ситуация с неиспользуемым кодом скорее всего всплывет.
А в случае с хорошим дизайном — «ненужным» может остаться только целый класс, который какую-то функциональность да выполняет, и который может потребоваться для своих целей в будущем. По крайней мере полного удаления он в данном случае не заслуживает
Я так понимаю, что readability review нужен только для того, что бы код соответствовал определенному стилю. А вы вроде говорите о code review.
UFO just landed and posted this here
Надо же, я думал, что Perforce — это тихий ужас, а его в Гугл используют. Видимо, я чего-то недопонял в нем.
Видоизменённый перфорс, называемый герфорс)

А статей-ка то древняя. Сейчас тут git набирает популярность потиху.
Open source продукты Google хранятся в svn.
code.google.com можно использовать с помощью Mercurial, а не только SVN
Код android доступен через GIT (+обертка под названием repo)
С помощью git'а можно пользоваться любым svn-репозиторием безо всякой обёртки. У меня такое чувство, что половины команды Хрома сидит с гитом.
UFO just landed and posted this here
git svn clone svn://example.com/projectname/
git svn fetch svn://example.com/projectname/

если я не ошибаюсь.
UFO just landed and posted this here
мне кажется это связанно с тем, что SVN имеет отличную от git идеалогию хранения истории (она линейна).

а разные url схемы это всего лишь разные способы доступа к существующему git-репозиторию.
UFO just landed and posted this here
UFO just landed and posted this here
Комментарий. Практически каждый сотрудник Гугла проходил readability review хотя бы по одному или двум языкам (тем, которые он использует в своём собственном проекте), так что, как правило, для каждого отдельно взятого коммита readability approval не нужно.
Комментарий к комментарию :)
В этом тексте словом «readability review» называется, натурально, code review, и просто сам факт его прохождения в одном коммите не избавит от прохождения redability review в последующих. А избавит от этого readability approval по профильному языку, который действительно имеет практически каждый инженер.
мне кажется в этом тексте «readability» это именно «readability», но интервью с Фитцпатриком цитируется все равно неверно. Дословно Брэд сказал

To check in you need three conditions met: You need someone to review it and say it looks good. You need to be certified in the language – basically, you’ve proven you know the style of this language – called “readability.” And then you also need the approval from somebody in the owner’s file in that directory. So in the case that you already are an owner of that directory and you have readability in that language, you just need someone to say, “Yeah, it looks good.”


А в тексте поста стало почему-то вместо «трех условий» — «два требования».

Кстати само интервью доступно в чуть ли не полностью в Google Books:

books.google.dk/books?id=nneBa6-mWfgC&lpg=PP1&ots=gDysIdRW0v&dq=Brad%20Fitzpatrick%20coders%20at%20work&hl=en&pg=PA49#v=onepage&q&f=false
Да, у Фицпатрега всё правильно написано.

«Нужно чтоб кто-нибудь посмотрел и одобрил изменения. Вам нужно иметь сертификат, именуемый „readability“, который показывает что вы знаете стиль языка. И вам нужно разрешение от кого-нибудь из хозяев каталога. В случае если вы сами хозяин каталога и у вас есть readability нужного языка, вам просто нужен кто-нибудь, кто скажет „выглядит отлично“.
Правильно оно тут называется LGTM — looks good to me
В данном случае это не для бюрократии, а всего лишь для порядка.
Конечно же и я именно об этом и говорю. Просто подобная система, да в плохих руках (а ведь всем известно, что у нас чего-чего, а плохих рук, а точнее мозгов — полно), поставленная через одно место, очень легко может перерасти в бюрократию. Просто представьте, вы нашли серьезный баг в коде, но не можете найти owner-а, поскольку он сегодня откосил, заболел или еще что-либо подобное. Либо вы не можете найти readability expert-а по haskell-у, поскольку он вообще у вас один на всю фирму.
Я как раз и выражаю респект за то, что ребята в Google не сделали из этого бюрократию.
А почему бы не использовать инструмент, который будет проверять соответствие кода стилю автоматически при коммите? Уж формат именования переменных или отступы несложно проверять автоматически.
И это тоже делается :) Но readability это чуть больше, чем правильные отступы
lint активно используется. И его замечания используются при ревью. Но делать соответствие lint'у или любому другому верефикатору формальным требованием — неправильно, так как всегда могут быть исключения. В стиле главное — здравый смысл.
Удивляюсь, как такая сложная процедура не оказывает чудовищное замедление на процесс разработки.

Или описанное актуально только для коммитов в чужие проекты?
Но ведь вас не удивляет наличие во многих конторах функционального (или design based) review кода перед каждым коммитом? А ведь подобное полноценное review гораздо более дорогостоящий, с точки зрения ресурсов, процесс. Поэтому для текущего проекта, скорее всего readability review просто является частью более строго review и все. Ну а с owner-ом в этом случае также проблем быть не должно (lead dev или pm, в целом кто-нибудь из команды обязательно будет owner-ом).
Пожалуй уточню, чтобы было понятней:

«In fact, they'll usually try to torpedo any changes that don't benefit them, and since they're the superstars, their opinions will carry more weight.»

Фактически, система ревью сама по себе неплоха, но меня в гугловой системе смущает то, что ревью проводится а) владельцем кода и б) ревьювером того проекта. Получаем, что при наличии «инженерной лестницы» (о чем как раз написано по ссылке) и в большой компании вполне проявляются эффекты вида «torpedo any changes that don't benefit them».

И такие вещи могут вполне сильно напрягать людей, те люди, которые талантливы, но не могут по разным причинам подвинуть владельцев кода и тимлидов (которые явно работают дольше и имеют больший вес в спорах) будут уходить.
Я сейчас (не в гугле, к сожалению), разгребаю код за человеком, который был, несомненно, талантлив и продуктивен, но, проработав около года, оставил за собой такой шлейф недокументированного кода, что сейчас для минимальных изменений, приходится, образно, аккуратно переносить карточный домик с места на место. Лучше бы на него был владелец кода/тимлид.

А вообще, описанная ситуация — типична для оупенсурс проектов (где никого не напрягает). Форкнуть код может кто угодно, а вот, что бы в апстрим закоммитить патч, будь добр, проведи определённую работу над ченджсетом.
Вы рассматриваете ситуацию, когда контроля вообще нет, а такие крайности конечно чреваты совсем другим уровнем косяков. Вопрос не в наличии на самом деле контроля со стороны владельца, вопрос в том — насколько много надо владельцу давать прав на блокировку изменений. ну и также вопрос в том — как построить процесс ревью, чтобы исключить негативный человеческий фактор со стороны тимлидов и владельцев _в условиях инженерной лестницы гугла_.

в статье сказано про стартапы, в которых проще банально из-за того, что там разработчики как бы это сказать… более равные :)
А вы представьте, что приходите вы на следующий день на работу и в вашей личной утилитке от вашего кода осталось только название файла. Или, скажем, вместо тщательно причёсанного понятного кода вы видите какой-нибудь «боярский» код, написанный «just for lulz».

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

> меня в гугловой системе смущает то, что ревью проводится а) владельцем кода и б) ревьювером того проекта.

Собственно, я показал, как отсутствие этого может привести к проблемам в развитии проекта.
P.S. Если вы предлагаете какой-то промежуточный вариант, при котором и талантливые энтузиасты будут «сыты» либеральностью приёма патчей, и владельцы проектов будут «целы» после того, как их транк загадят гении, то предложите его.
Фактически, система ревью сама по себе неплоха, но меня в гугловой системе смущает то, что ревью проводится а) владельцем кода и б) ревьювером того проекта. Получаем, что при наличии «инженерной лестницы» (о чем как раз написано по ссылке) и в большой компании вполне проявляются эффекты вида «torpedo any changes that don't benefit them».

Когда любой человек может править любой код, владелец проекта может в один прекрасный день заметить, что его код валится на половине тестов, безбожно тормозит и вообще делает не то, что нужно. Так что в любом случае держать владельца в курсе всех изменений — хороший тон. Тем более, что он, как правило, лучше всех ориентируется в архитектуре и может выявить косяки еще на стадии code review.
как мне кажется вы смешиваете несколько разных вещей:
— «валится на половине тестов» — решается периодическим прогоном этих тестов и раздачей пряников тем кто внес изменения которые их валят, это вообще сильно техническая вещь.
— безбожно тормозит, делает не то, что нужно — вопрос контроля разработчика со стороны тимлидов или тимлидов со стороны менеджмента

а вот ревью кода это ближе именно к стилю написания, но не только, наверно. собственно сам термин (readability review) врядли относится к тому, что код падает на тестах :)

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

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

приятно конечно работать с понимающими людьми, но в случае с гуглом, а он уже стал большой компанией и давно перестал быть стартапом, подобные сайд эффекты начинают уже влиять на разработку. в стартапах же такое решается (или не решается, в отдельных случаях типа моего) как правило вменяемым средним менеджментом и как правило просто незаметно.
решается периодическим прогоном этих тестов и раздачей пряников тем кто внес изменения которые их валят, это вообще сильно техническая вещь.</>
Универсального решения нет. Очень многие новые фичи могут оказаться бомбой замедленного действия, которая сработает не сразу, а после какого-нибудь безобидного коммита, который и будет автоматически занесен в blame-list.

безбожно тормозит, делает не то, что нужно — вопрос контроля разработчика со стороны тимлидов или тимлидов со стороны менеджмента
Ну зачем так сурово? Я просто имел в виду возможность внесения performance regression. Это, понятное дело, тоже должно ловиться автоматически, но на деле часто бывает, что можно обратить внимание и на стадии code review.

а вот ревью кода это ближе именно к стилю написания, но не только, наверно. собственно сам термин (readability review) врядли относится к тому, что код падает на тестах :)
Возможно, это не очень точно отражено в посте, но разрешение от владельца кода ничуть не менее важно, чем readability approval. Потому что владелец кода обязательно старается отследить возможные проблемы, которые появятся после коммита.
решается периодическим прогоном этих тестов и раздачей пряников тем кто внес изменения которые их валят, это вообще сильно техническая вещь.
Универсального решения нет. Очень многие новые фичи могут оказаться бомбой замедленного действия, которая сработает не сразу, а после какого-нибудь безобидного коммита, который и будет автоматически занесен в blame-list.

безбожно тормозит, делает не то, что нужно — вопрос контроля разработчика со стороны тимлидов или тимлидов со стороны менеджмента
Ну зачем так сурово? Я просто имел в виду возможность внесения performance regression. Это, понятное дело, тоже должно ловиться автоматически, но на деле часто бывает, что можно обратить внимание и на стадии code review.

а вот ревью кода это ближе именно к стилю написания, но не только, наверно. собственно сам термин (readability review) врядли относится к тому, что код падает на тестах :)
Возможно, это не очень точно отражено в посте, но разрешение от владельца кода ничуть не менее важно, чем readability approval. Потому что владелец кода обязательно старается отследить возможные проблемы, которые появятся после коммита.
вы все верно говорите, но я писал о той проблеме, когда владелец кода торпедирует внесение изменений руководствуясь не критерием «лучше для проекта», а критерием «а что это даст лично мне».

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

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

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

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

«как правило разбирается» еще не означает «разбирается всегда лучше». а из этого следует, что если приходит новичок (в компании, но с хорошим бекграундом вне нее), видит явный косяк в архитектуре, пытается сам его поправить, правит, а код откатывается тимлидом со словами «я разбираюсь лучше» и все попытки обсудить архитектуру спускаются им же, ссылаясь на свой заслуженный авторитет, то такая ситуация решается только на уровне менеджера проекта с привлечением сторонних специалистов и ревью кода тут ну никак помочь не сможет, к сожалению :(
Чуток попридираюсь к словам, чтоб не плодилось ложных мифов: груплетом называется не само 20% время, а его использование в целях, не связанных напрямую с кодированием в каком-то проекте, а направленных на улучшение чего-либо (например тестирования) в масштабах компании:

But when the thing you really want to work on is to make a broad change across the whole organization, you need something new — you need a “grouplet.”
Честно говоря, бралось отсюда: www.googleapps.ru/2007/11/google-apps.html. Поскольку пост про Google, то решил, что сам Google не будет против, если в посте про него самого будет его же картинка (ух!) :)
Но поскольку, судя по всему, именно вы являетесь автором упомянутого мною выше ресурса, то спасибо вам за рисунок. Если спасибо — «это слишком много», ну что же, тогда придется подбирать другой рисунок:)
С самого начала скажу что я только за использование своих иллюстраций. Приятно видеть, что они нужны и полезны людям. Если нужно, могу выслать исходник в psd. Просто при первом взгляде мелькнула в голове мысль, что нет ссылки на источник, потом, поразмышляв понял, что Вы внесли в изображение изменения и наверное не очень логично ставить ссылку на первоначальный вариант.

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

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

а исходники… да кому они, если честно, нужны без людей, которые знают что там написано и людей, которые стояли рядом и занимались маркетингом, анализом требование и прочее и прочее :)
Прям уж всех проектов исходники доступны :)
>>кстати, совершенно не обязательно, чтобы это были два разных человека, две эти роли вполне могут ужиться и в одном >>человеке
Рискну предположить что Guido van Rossum — readability approved person для питона в гугле ) ну наверняка.
в тему общего репозитория, однажды я задавал примерно такой вопрос одному из разработчиков гугловолны:

Nov 19, 2009 Me:
...your issue tracker has internal ids over 2000000.
do you realy had so much issues tracked?

Nov 19, 2009 Austin Chau:
The internal tracking id is something we use across all google project.
as you can imagine, there are quite a few of those :)
То то меня поразило, когда один из гуглеров в Wave начал сразу мой код (семпл) бодро править.
А потом написал LGTM и только тогда обсудили что же этот код делает.

Спасибо автору за пост и сообществу за обсуждение.
> Анализ кода (readability review), который проводится экспертом по соответствующему языку призван обеспечить прежде всего согласованный стиль написания кода, ведь для любого крупного проекта гораздо важнее единство стиля и минимальное количество «переключений» мозга при чтении с одного стиля на другой. В этом случае на первый план выходит не абсолютная полезность некоторого правила, а его согласованное применение (да, лично я предпочитаю другой формат отступов или формат именования переменных, но гораздо лучше, чтобы стиль форматирования кода был единым, независимо от сотен различных предпочтений различных сотрудников).

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

Может кто-то может поделиться опытом? Очень хочется делать review проектов своей команды.
Некоторые соглашения об оформлении кода доступны открыто (С++, Objective-C, Python): code.google.com/p/google-styleguide/

Для автоматической проверки стиля (там где это возможно) используются разного рода lint-еры, например, google-styleguide.googlecode.com/svn/trunk/cpplint/

Для организации ревью используется тулы типа Rietvield (http://code.google.com/p/rietveld/, используется на codereview.chromium.org/) и Mondrian (techtalk: www.youtube.com/watch?v=sMql3Di4Kgc).
«каждый человек может получить исходники любого проекта в любой момент времени» — как они не боятся, что кто-то из сотрудников скопирует себе весь исходный код гугла себе и потом кому-нибудь продаст, передаст или выложит в инет?
Sign up to leave a comment.

Articles