レビュー会のすすめ

先週日本にインドネシアのプロダクトマネージャーが来た時に「レビューに時間がかかりがち」「結果として開発のリードタイムが予測しづらくなっている」という悩みを相談してくれた。そのときに「コードレビューを会話しながら行う取り組み - Hatena Developer Blogとかどうですかね、日本のチームでも “レビュー会” ってのをやっていますよ」と伝えようとしたが日本語の記事を渡すのも気が引けたので「あとでまとめておく」ってことにして社内向けに “Encouragement of Review Meeting” という snippet を書いた。

これを日本語でも書き直してみる。

---

背景

最近プロダクトマネージャーと開発者間でレビューに関してこんな感じの問題があるらしい。

~ ある日 ~

マネージャ「進捗どう？」

開発者「だいたい終わってレビュー待ちの状態かな」

マネージャ「オッケー」

~ 別の日 ~

マネージャ「どう？Pull Request はマージされた？」

開発者「いや、それがまだで…。もう一回 ping してみる」

マネージャ「そうか…どれぐらいかかりそう？」

開発者「うーん、わからん…」

~ 以下、ループ ~

---

開発者による見積もりがすぐに元々の数字から変わったり、リリース日がずれたり、誰にとっても嬉しくない状況である。

一方、日本の開発者は複雑だったり大きすぎたりする Pull Request を作ってしまったときには “ペアレビュー” や “レビュー会” を設けており、今のところうまく機能している。

前提

そもそもレビューのリードタイムが長くなるのは Pull Request がでかいか複雑か、それに見合う説明が足りてないか、だ。

まず開発者が意識すべきなのは

• Pull Request を適切なサイズに分割すること
• 充分な説明、図、wiki などを書くこと

とはいえでかい Pull Request を作らざるを得ないこともあり、得てしてそれは長い間放置されがちである…。

詳細

そうした状況を避けるため、日本の開発者は “レビュー会” というのをやっている。

1. レビューを求める開発者（レビュイー）はまず Pull Request （必要なら wiki なども）を前もって用意し、ミーティングを予約する
   • 全員が主体的に参加できるよう、レビュワーの数は2 ~ 3人がベストだと思う
2. レビュイーはミーティングで Pull Request や wiki や図などを見せながら説明する
   • たまにすごく絵を書くのが上手い人にでくわす
3. レビュワーはいつでも何でも質問して良い
   • たまに激しい意見が飛び交うかもしれないが落ち着いて欲しい
4. 質問されたこや議論の内容は GitHub の review comments として残す
   • 残さないとあとでわけがわからなくなる
5. ミーティングが終わったらコードを修正する
   • 結構な量の修正になったとしても指摘内容や実装意図を理解しているので再レビューはかなり簡単

Note

• 全ての Pull Request がこうした取り組みを必要としているわけではない
• “ペアレビュー” も “レビュー会” もコミュニケーションツールだということを忘れないこと。普段レビューを円滑にするために行っていることをそのままやるとよい
  • レビュイーは対面だからとか時間を取ってしまうからと気兼ねせずレビュー依頼や質問をすること
  • レビュワーは良いところは褒めつつレビューされたし

終わり

---

---

Original post:

Background

I recently heard some @quipper/product-managers (or even developers) are troubled with reviewing time. Is the situation like below?

~ One day ~

Manager: “How is your work going?” Developer: “Almost finished, but… it still needs to go through review.” Manager: “Understood.”

~ Another day ~

Manager: “Did your PR get merged?” Developer: “Ah, not yet. I’ll ping them again.” Manager: “Hmm, I see. How long does the review take?” Developer: “Umm, not sure.”

~ To be continued… ~

---

It’s so frustrating for everyone. Estimation by a developer easily varies from the original one, and its release date does.

On the other hand, Japanese developers sometimes have kind of “pair reviewing” or “review meeting” for a complicated (large) pull-request in person. So far, it works so well that I’d like to share the way.

Premises

Most of the review time issues are caused by a large/complicated pull request, and lack of adequate description.

So first of all, developers should

• split pull-requests into adequate size ones if possible
• write an adequate description, chart, wiki etc. for a feature they develop

Even though the idea above is right, we are sometimes compelled to implement a large/complicated pull request. And it will have been neglected for a long time…

Details

To avoid such situation, Japanese developers sometimes have “review meeting”.

How?

1. A reviewee who needs a review prepares PR and wiki (if necessary) beforehand, and book a review meeting
   • 2 or 3 is a good number of reviewers. If it’s too much, all of them might not actively join a discussion.
2. The reviewee explains his/her implementation by showing PR, wiki, chart etc.
   • I know some devs are quite good at to write a chart.
3. A reviewer can ask/discuss anything anytime in the review meeting
   • Be calm, it’s not an interview.
4. Questions/Discussion points are noted as a review comment on GitHub
   • If we skip this step, some points will be missed.
5. After that, the reviewee reflects the content of the review meeting
   • Even if there are so many fixes, it’s much easier to re-review since reviewers know why those changes are needed.

Note

• Not all of pull-requests requires “review meeting” and “pair review”.
• They are just communication tools, so
  • If you are a reviewee, don’t be afraid to ask a review.
  • If you are a reviewer, it’s quite encouraged to pay a high compliment to one’s code.

END

プロダクトの複雑さと引き算の重要性

最近「それ開発しなくてもよくないか…？(既存機能の組み合わせとか別の道がありそう)」と思う案件多い気がしていたが、自分がそういう観点でレビューしてるんだと思う

— 広島の粗大ゴミ (@ohbarye) 2017年6月28日

コードを書く気持ち

「開発しなくても既存の機能や基盤のうえで提供できる価値はあるし、既存機能を組み合わせて新商品や機能を作り出すこと、またはフィジビリティを行うことも場合によってはできる」…なんだかそういうことを常に念頭において開発するようになり、とにかくコードを書ければそれでよいみたいな気持ちがなくなってきた。

もちろんずっとコードを書いていたいのだが、小さい問題解決のために大きな問題を後回しにするようなコードは書きたくない…という迷いがある。

携わってるシステムがそこそこ大きく、一部はレガシーと呼んでも良さそうな年季が入っているので、無策でこれ以上コードを積み上げていくのに少なからず恐怖心もある。

複雑なるもの

なぜ迷いや恐怖心があるのか。複雑なものの上に新しい何かを載せてできるのは「ちょっと複雑なもの」ではなく「かなり複雑なもの」になってしまうからだ。

てきとうな式だと「複雑度10 + 複雑度1 = 複雑度15」 みたいなイメージだ。

大げさに言っているようだがこれはあながち間違ってもいなさそう…　というのも、最近参加したメンバーからのフィードバックで「当初の見積もりより開発に4倍時間がかかった」というものがあり、理由を聞いたところ「既存の仕様や実装との兼ね合いが開発途中で判明し、当初に想定していたシンプルなものではなくなってしまった」という。

「見積もりが悪い」と責めるのは簡単だが俺はそうは思っていなくて「見積もりが正確にできないほど複雑なものを相手にしている」というのをチームの共通認識として持つべきだと思う。

立ち向かう案

「複雑だからしょうがない」と言って諦めてもしょうがない。

複雑だからこそ新しいものを1つ足すときには引き算の考えも同時に欲しい。最低でも1つ、可能なら2つでも3つでも同時に引き算することで複雑さを減らしていくしかない。

「何も引くことができない」というような状況の場合、振り返りや分析がきちんとできていない可能性を疑いたい。足す時に理由が必要なように、引くのにも理由が必要だからだ。

もし誰もそのことを気にかけていないなら、足し引きのバランスを担ったり引く理由を追うためのチームがあっても良いぐらいだと思う。

JavaScript や CSS を読み込む時の type 属性はもはや不要

kechol 氏からレビューを受けて知ったのでメモ。

https://google.github.io/styleguide/htmlcssguide.html#type_Attributes によると CSS や JavaScript を読み込むときのタグに type attribute は HTML5 では不要、というか非推奨とのこと。

引用

Omit type attributes for style sheets and scripts.

Do not use type attributes for style sheets (unless not using CSS) and scripts (unless not using JavaScript).

Specifying type attributes in these contexts is not necessary as HTML5 implies text/css and text/javascript as defaults. This can be safely done even for older browsers.

<!-- Not recommended -->
<link rel="stylesheet" href="https://www.google.com/css/maia.css"
  type="text/css">

<!-- Recommended -->
<link rel="stylesheet" href="https://www.google.com/css/maia.css">

<!-- Not recommended -->
<script src="https://www.google.com/js/gweb/analytics/autotrack.js"
  type="text/javascript"></script>

<!-- Recommended -->
<script src="https://www.google.com/js/gweb/analytics/autotrack.js"></script>

---

こういうの、手癖で何も考えず書いたりしてしまう割に頻繁に情報アップデートしてもいないので Google HTML/CSS Style Guide を一通り読んでおいたほうが良さそうだ。

改行をレスポンシブにコントロールする

スマートフォンで閲覧する時は改行してほしい。PC では改行してほしくない。

みたいなデザインを実装している時に、同僚の @kechol 氏にレビューで教えていただいたのでメモ

See the Pen Responsive Line Break by Masato Ohba (@ohbarye) on CodePen.

CSS のメディアクエリで改行タグの display スタイルを操作すればよい

Clojure で初めての unless マクロを書く

『7つの言語 7つの世界』を読みつつ Clojure 入門している。その中でマクロに関する記述があったが LISP のマクロに馴染みがないので、コードを見ても何が起きているのか瞬時にわからなかった。

正解のコードを少しずついじりながら、なぜそう書かないといけないのか、なぜそう書くと動くのかを確かめてみた。

ちなみに例は unlessで、これはマクロ入門でおなじみらしい。

失敗例

まずは挙げられているダメな例から。

(defn unless [test body] (if (not test) body))
#'user/unless

(unless true (println "Lose Yourself"))
Lose Yourself
nil

body が評価されてしまっている。test が false のとき以外は評価してほしくない。なのでこれがダメなのはわかりやすい。

正規順序でなく作用的順序で評価してしまうと無限ループが発生してしまう例が SICP にもあった（気がする）。

正しい unless

以下のようになるようだ。

(defmacro unless [test body]
  (list 'if (list 'not test) body))

これをいじってみる。

if を即時評価していないのはなぜ

'if の部分は以下のように書けるのではないだろうか？

(defmacro unless [test body]
  (if (list 'not test) body))

(unless true (println "Nothing to tell."))
Nothing to tell. ;; 評価されてしまっている
nil

(unless false (println "This is it."))
This is it.
nil

はい、body が評価されてしまっているダメ。

(macroexpand '(unless cond body))
body

上記の unless をマクロ展開してみると body を実行するだけのマクロになってしまっていることがわかる。

not を即時評価していないのはなぜ

では、'not の部分は以下のように書けるのではないだろうか？

(defmacro unless [test body]
  (list 'if (not test) body))

(unless true (println "Nothing to tell."))
nil

(unless false (println "This is it."))
This is it.
nil

一見成功しているように見える…！

だがそれが命取り…！実は駄目…！

上記の unless をマクロ展開してみると条件部が既に評価されてしまっていることがわかる。

(macroexpand '(unless condition body))
(if false body)

;; 条件は false になるのに println が実行されない
(unless (= 1 2) (println "This is it."))
nil

正しい unless をマクロ展開すると条件部がまだ評価されていないことがわかる。

(macroexpand '(unless cond body))
(if (not cond) body)

;; ちゃんと unless 呼び出し時に評価されている
(unless (= 1 2) (println "This is it."))
This is it.
nil

defn で書いたらどうなる？

(defn unless [test body]
  (list 'if (list 'not test) body))

(unless true (println "Nothing to tell."))
Nothing to tell.
(if (not true) nil)

(unless false (println "This is it."))
This is it.
(if (not false) nil)

(class (unless false (println "This is it.")))
This is it.
clojure.lang.PersistentList

(list 'if (list 'not test) body) が評価されてしまった後のリストが返ってくる。body も評価されてしまっているので println が実行されている。

関連する関数などの知識

quote

評価してほしくないフォームに quote 関数を適用すると評価を抑制できる。

ダメなマクロの例を成り立たせるために body を' で遅延評価させられる。もちろん呼び出し側でコントロールしているのでよくない。

(defn unless [test body] (if (not test) body))
#'user/unless

(unless true (quote (println "Lose Yourself")))
nil

;; ' 記号でも書ける
(unless true '(println "Lose Yourself"))
nil

defmacro

defmacro は defn のように関数を定義するが、その戻り値は実行時にコンパイラにマクロとして解釈されるようになる。

(doc defmacro)
-------------------------
clojure.core/defmacro
([name doc-string? attr-map? [params*] body] [name doc-string? attr-map? ([params*] body) + attr-map?])
Macro
  Like defn, but the resulting function name is declared as a
  macro and will be used as a macro by the compiler when it is
  called.
nil

;; 確かに Macro と解釈されているようす
(doc unless)
-------------------------
user/unless
([test body])
Macro
  nil
nil

---

7つの言語 7つの世界

『グランドイリュージョン』（原題: Now You See Me）観た

あらすじ

ダニエル・アトラスら4人の男女で構成されたマジシャンチーム“フォー・ホースメン”がラスベガスでショーを行うのと同時にパリの銀行から金を盗み出すという大技を行う。FBI特別捜査官のディラン・ローズとインターポールが彼らの犯罪を阻止しようとするが、失敗して途方に暮れ、マジックの種明かしの名手サディアス・ブラッドリーに助けを求める。 グランド・イリュージョン (映画) - Wikipedia

良かったところ

映像美もあるし題材ならではの謎解き感もあるし、何よりクライムサスペンスなので全体的にスカッとして良かった。テンポもよい。

4人組のキャラクターもそれぞれ魅力的だし俳優陣も良い。特にゾンビランドのコロンバスとタラハシのコンビの再演は熱い。

惜しいところ

原題

マジシャンの常套句を使っている原題の方がどう考えても格好良いと思う。

というか “Now you see me” (見えますね) を使わないと作品の中の “Now you don’t!” (はい消えた！) が通じなくなるというか、軽くなってしまうような…。

CG過剰

「いやぁーそれは無理でしょ」と突っ込みたくなるようなショーマジックを観ていると「うーん、映画だなぁ」と一歩引いてしまう。実際のマジックも「いやぁー無理でしょ」というものなんだろうけど、ライブ感ゆえにそこに注意はいかない（"近づくほど見えなくなる"から）。

映像の面白さとしては"有り"だし、必然性があってそう描かざるを得ない"映画的な嘘"については物凄く好きなのだが、一歩惜しかった印象。

5月は短期集中して賃貸物件探しを終わらせた

年初に 2017年の目標 -1年の目標を立てるのをやめる- - valid,invalid で書いたように今年は30日ずつ何かを頑張ることにしており、5月は主に物件探しに打ち込んだ。

物件探しは妥協したくないのだが仕事の片手間にやるのもなかなか大変だ。物件検索サイトをずっとウォッチしているのもストレスになるし、探す期間に終わりが見えないと辛い。というわけで5月にもろもろ片付ける勢いで頑張った。

目標

• 満足のいく賃貸物件を見つける

結果

以下の記事にまとめた通り、条件に見合う良い物件が見つかった。

ohbarye.hatenablog.jp

条件には書かなかったが「光回線インターネット無料」「ミストサウナ付きバスルーム」など予想の斜め上の特典も付いてきて嬉しい。

感想

ブコメにも「賃貸物件探しによくそんなにリソース割いたな」とあったけど、自分でもそう思う。1ヶ月で集中して終わらせるという気持ちを最初から持って臨んだおかげでなんとかなったと思う。

バズったこと

引越しの記事が思っていた数十倍バズってて驚いてる
言語学習（英語）とか引越しとか、社会性動物の尽きない悩みなので内容同じでも何度でも焼き直せそうな感じはあるな

— 広島の粗大ゴミ (@ohbarye) 2017年6月11日

PV / はてなブックマーク数の比が技術系の記事でバズったときより圧倒的に大きいのでどうやら"異方"に届いているようだ

— 広島の粗大ゴミ (@ohbarye) 2017年6月11日

ブログに入れた Google Analytics が初めて見ていて面白い感じになった。SmartNews からも来てるのか pic.twitter.com/AtNMxsNopN

— 広島の粗大ゴミ (@ohbarye) 2017年6月11日

ヨッピーさんの下に自分の名前あって光栄 pic.twitter.com/PIyvuiUsf6

— 広島の粗大ゴミ (@ohbarye) 2017年6月12日

1100強ブクマであわせて22000PV、このブログ1年分のアクセスだ…。

純粋に嬉しい。

が、今日職場でプロダクトマネージャーと卓球しながら雑談していたら「あんな内容でバズるんですね？」と煽られた。彼もまた引越しのベテランのようなのでそのうち知見を公開して欲しい（見積書みたいなのを作って業者に埋めさせるとか言ってた）。

その他

ちょっと楽器を弾きたくなったので Victor Wooten の “Sometimes I Laugh” を頑張ってコピーした。

スタジオ版だとベースが何本も重なっているが、1本で弾けるようアレンジしているベーシストが Youtube で何人か見つかる。その中で最もかっこよかったこの人のアレンジをコピった。

途中で不可能ぽい運指があると思ったら12フレットのハーモニクスで音を繋いでいる箇所（0:49）があってけっこう感動した。