トランザクション内で行うべきことは更新処理のみ

トランザクション内で行うべきことは更新処理のみ

どうしてそう思うのか？

  • トランザクションはアトミックな処理を保証するもの
  • 一方でアトミックでない処理にならない様にロックをかける
  • 場合によっては別ユーザにLock wait timeoutの被害を与えるなんてこともありえます

どうすればよかったのか？

  • トランザクションを開始する前にすべてのREAD処理を終わらせる
  • 依存するIDがわからない場合にlastInsertIdを取得するといった行為はその限りではありません
  • ORMを使っていれば、依存するエンティティのIDを使えば無意識にできますね

伝えたいこと

目的と効果を明らかにする

目的と効果を明らかにする

  • レビュー依頼は依頼だよねという話

依頼時あるある

口頭でお願いされる

  • 少なくともチャットなりのテキストに残しましょう
  • 障害対応のhotfixなら仕方ないけど、それでも後でテキストに残しましょう

お願いしますだけ

Aさん: これレビューお願いします
Bさん: 何を見たらいいの。。

差分に見合わない簡単な説明

  • これは先日あった実話
  • 依頼を受けたのは別の人間だったが、どの様に対応したか確認のためにチケットまで見たが対応内容は書かれていなかった
Aさん: 高速化対応をしましたレビューお願いします
  • この時、期待していた情報は下記のもの
  • 重い処理とそのベンチ結果
  • その処理のパフォーマンスが悪い原因
  • どう直すことでよくなるか、もしくはよくなったか
  • Before/Afterのベンチ結果

レビュアーの目的を明らかにする

  • 改善活動であれば何をしたいのか、何がよくなるのかを依頼時に明らかにする
  • 問題の解消であれば原因とその対応内容を書く
  • 仕様に関しての補足が必要であればそれも書く
  • パフォーマンスの向上を図ったのであればベンチ結果を書く
  • 究極的にはコードを見なくてもどいうコードか想像できる依頼文を目指す

伝えたいこと

  • コードレビューはおつかいではない
  • 依頼文はお願いでなく何をして欲しいかを伝えるもの
  • 相手の負荷を下げ、相手に理解してもらえる依頼を意識しよう

何でこう思うのか？

  • プログラミングはコンピュータにやって欲しいことを伝えること
  • レビュー依頼は人間に見てもらいたい内容を伝えること
  • 伝達という意味において両者は等しく、レビュー依頼の精度を上げることはコードの品質向上にもつながる
  • 何故、何をどうやったか、してほしいかが逆転しているが両者は等しい関係である
  • よいコードを書くためにも正確なコミュニケーションを常に心がけよう

長いPRは避ける

長いPRは避ける

  • 新規開発とかをしていると起こりがちなのが数千行にも及ぶ差分
  • 数千行ものコードを正しくレビューできる人間なんてそうそういない
  • 少なくとも自分は何かを見落とす自信がある

どうすればよかったのか？

  • 機能ごとの開発に切り分ける
  • それでも差分が大きくなりそうであれば、以下の様な単位に切り分ける
  • 画面遷移などのロジックを伴わない処理
  • 使うであろうDBやキャッシュ、ストレージへのアクセス
  • ビジネスロジックの簡単な作り
  • 見てもらい単位で関数ごと、もしく関数群
  • バリデーション
  • 異常系の漏れがないかのチェック

何が嬉しいか

  • 何か根本的に間違ったコードや設計があった場合に早期発見が可能になる
  • レビューする側が仕様を正しく認識していない場合は仕様の細かい点まで理解しやすい
  • どういうI/Oになっているのか？
  • どんなSQLが発行されて、それらはパフォーマンス劣化を起こさないか？
  • バリデーションに漏れはないか？適切なバリデーションが行われているか？
  • レビューのためのコンテキストスイッチの切り替えは大変だが、20−30min程度であれば意外と息抜きになる

注意したいこと

  • 細かすぎても逆にストレスを与えるかも知れない
  • 実際にやってみて、合意がとれてから行う方がよいだろうとは思う
  • 合意がとれなかったことは今までになかったが
  • こうしましょうねと見せても相変わらず巨大な差分を生み出す人間はいたが。。

どうやるか

  • 目安は200−300行程度を上限にする
  • 見てもらいたい単位を設計時に明らかにしておく
  • レビュー観点を満たすだけのコード単位でコミットすることは簡単
  • また、その単位を習得していくことによって設計等の考え方も日々、変わってくる
  • 一つのことに集中してレビューしてもらえる様に保つ

不等号の向きは合わせる

不等号の向きは合わせる

  • こういう複合条件の場合に頭の中で反転させることをなくしたい
if ($i > 1 && $i < 100) {
    // 何らかの処理
}
  • この様に書くと直感的になる
if (0 < $i && $i < 100) {
    // 何らかの処理
}
  • これもPBP(Perl Best Pracitice)に書かれていたものだと思う。
  • ちなみに、読んでしばらくは左辺に定数を置くことでバグを避けるということもしていた
if (10 = $i) {
    // 何らかの処理
}
  • 現在ではこの左辺に定数リテラルを書く記法に関しては行っていない

否定のELSEは基本的に避ける

否定のELSEは基本的に避ける

  • こういうのを見るとモヤモヤします
if (!empty($foo)) {
    // 正常処理
} else {
    throw new \Exception('foo is empty');
}

理由

  • !emptyで空でない時という条件を指定しておきながら、逆説のELSEをとってしまうところ
  • たしか、これに違和感を覚える様になったのはPBP(Perl Best Practice)の影響かな？

備考

  • ただし、正常処理が少し長くなる場合に異常系を先に見せるとかであれば許容します