Яков Сироткин ([info]yakov_sirotkin) wrote,
@ 2009-06-24 15:14:00
Previous Entry  Add to memories!  Tell a Friend  Next Entry
Entry tags:refactoring

Рефакторинг и code review
В некоторых компаниях есть практика code review, вплоть до того, что если один программист хочет сделать коммит в репозиторий, то он оформляет его в виде патча, посылает коллеге для утверждения, а коллега присылает свои замечания. И, пока недочёты не исправлены, коммит не делают. Те изменения, которые делаются на основе code review — это классический рефакторинг.

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


(17 comments) - (Post a new comment)


[info]shisha_hwguy
2009-06-24 11:43 am UTC (link)
Code Review дело очень хорошее и правильное, но пока удобных тулов для него я не видел.
Зачастую его использование превращается в пытку.
Особенно для большого проекта.
Особенно когда есть четкие сроки.

(Reply to this) (Thread)

[info]seliv
2009-06-24 01:03 pm UTC (link)
Atlassian Crucible ?

(Reply to this) (Parent)

[info]kmmbvnr
2009-06-25 11:06 am UTC (link)
Что-то где-то читал про тулзу применяемую в VMWare. Контролирующее code review каждой строчки diff'a

(Reply to this) (Parent)

[info]display_none
2009-06-24 01:07 pm UTC (link)
У нас делают кодревью, и в этом еще есть немного от парного программирования.
Хотя мы и не используем XP, но ощущение того, что этот код придумал не только ты (кто-то его еще смотрел) как-то успокаивает.

(Reply to this)


[info]ivan_ghandhi
2009-06-24 02:58 pm UTC (link)
Толку-то. Друзья между собой ревьюят, другим палки в колёса ставят. Можно подумать, таким образом можно научить кого-то программировать. Или что продуктивный китайский программист будет рефлексировать по поводу трижды повторённого, в трёх разных методах, одной и той же комбинации ифов и свичей. "Так надо" - вот и всё.

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

(Reply to this) (Thread)

[info]yakov_sirotkin
2009-06-24 08:49 pm UTC (link)
Я не агитирую за code review, я сам его делать не умею:)

(Reply to this) (Parent)

[info]8f4a4bdd981a
2009-06-24 08:28 pm UTC (link)
"Те изменения, которые делаются на основе code review — это классический рефакторинг.

Соответственно, рефакторинг — это подготовка к code review."

Commit как-то выпал из этого замкнутого круга. :-)

(Reply to this)


[info]8f4a4bdd981a
2009-06-24 08:29 pm UTC (link)
Пока не придет ответ от коллеги, код все равно надо куда-то закомитить.

(Reply to this)


[info]cheatex
2009-06-24 09:09 pm UTC (link)
Меня очень давно подмывает ввести в прокте такую практику с обзором патча перед вливанием в голову. Но боюсь, что коллеги в итоге порешат :(

(Reply to this)


[info]provokatorz
2009-06-25 07:36 am UTC (link)
"В некоторых компаниях есть практика code review..."

А можно пару примеров компаний где есть "практика code review"?

(Reply to this) (Thread)

[info]rusborland
2009-07-21 12:18 pm UTC (link)
nvidia

(Reply to this) (Parent)

[info]rusborland
2009-07-21 12:21 pm UTC (link)
Да, в принципе, все крупные компании имеет практику code review на тот который хотя бы как-то влияет на продукт, и на productivity tools.

Другие примеры это google, microsoft.

(Reply to this) (Parent)

[info]d_zh
2009-06-25 01:52 pm UTC (link)
У нас была практика code review более трех лет. Прямо в Jira после проверки задачи тестированием следующий статус был ReadyForCodeReview, по результатам которого (да/нет) задача либо считалась сделанной и закрывалась, либо отправлялась на переработку.

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

(Reply to this) (Thread)

[info]yakov_sirotkin
2009-06-25 02:32 pm UTC (link)
У нас приходят diff по сделанным коммитам и у меня есть коллега, который на их основе может делать грамотные замечания.

Гигиену поддерживаем при помощи жёлтых маркеров в IDEA.

(Reply to this) (Parent)(Thread)

[info]d_zh
2009-06-25 06:50 pm UTC (link)
А можно пару примеров грамотного замечания? Если коммитящий не совсем бегинер, я очень сомневаюсь, что беглым просмотром изменений можно заметить что-то, что не заметит code inspection.

(Reply to this) (Parent)(Thread)

[info]yakov_sirotkin
2009-06-25 07:27 pm UTC (link)
Например, я закоммитил добавление колонки не в ту версию скрипта для апдейта базы. По счастливой случайности это не привело бы к проблемам, но я всё равно идиот. А бывали случаи, что code review ловил баги, которые 100% прилетели бы к нам из QA. Иногда оказывается, что на самом деле всё правильно или что разногласия носят характер священной войны по мизерному поводу.

Разумеется, code review - это совсем не "беглый просмотр", для него мозг нужен. Я вот, например, не умею code review делать, даже на TopCoder всего пару раз находил ошибки в чужих решениях.

(Reply to this) (Parent)(Thread)
Полезно для здоровья
[info]kodla
2009-08-03 05:58 pm UTC (link)
Когда я только пришёл и попал к менеджеру, стремящемуся всё делать правильно, он устраивал нам code review после некоторых этапов проделанной работы. На мой взгляд, это очень хорошая практика в среде молодых специалистов, поскольку меня отучили от многих глупостей, которые я допускал по невнимательности: забывал про StringBuffer в цикле, обходил Map-у не через entrySet() и т.п. Хотя я прекрасно знал, что так не правильно, но часто допускал такие глупости.
С возрастом степень доверия растёт и code review можно делать реже, но всё равно нужно. Недавно при ревью кода одного специалиста с куда большим стажем, чем мой нашёл устрашающие страшилки. Да, и 100% баги мы тоже, кстати отлавливали. Но для этого надо подходит к этому процессу действительно серьёзно, а не как к очередной бюрократической процедуре.
Итог: с возрастом ответственность растёт, а следовательно и растёт уровень, на котором допускаются ошибки (человеческий фактор всё-таки). А code review по большей части полезен для начинающих.

(Reply to this) (Parent)

(17 comments) - (Post a new comment)

Create an Account
Forgot your login or password?
Login w/ OpenID
English • Español • Deutsch • Русский…