miso_soup3 Blog

主に ASP.NET 関連について書いています。

Google's Code Review Guidelines を読んでアクションに落とす

Summary

Google's Code Review Guidelines を読み、具体的にどうアクションするかを念頭に置くため、アクションリストだけをまとめました。

レビュアー向けのガイド

The Standard of Code Review 基準
  • プルリクのコードが完璧でなくても、確実に改善するのであれば、積極的に承認する。
  • 重要でない指摘は、”Nit:” をつけることで、変更するか否かは作者に委ねることを伝える。
  • コードレビュー上のコメントでのやりとりが難航しそうなとき、対面のミーティングやビデオ会議を行う。
    • そのとき、他のレビューワーのために、議論の結果はコメントとして記録する。
  • 合意を得られないからといって、プルリクを放置しない。
What to Look For In a Code Review 何を見るか
  • ユーザーへの影響を理解するにはコードだけでは分からないこともあるので、動作確認を行う。
    • 難しい場合は、作者にデモをお願いする。
  • オーバーエンジニアリングに警戒する。
  • 適切なテストを求める。
  • コード中のコメントがWhyであるか、Whatになっていないかを確認する。コメントが必要なものかを確認する。
  • 個人的なスタイルの好みに基づくものは“Nit:”とする。
  • 関連するドキュメントのアップデートを依頼する。
  • すべての行を読み、自分で理解する。
    • コードを読むのが難しい場合は、明快にするよう作者に伝える。
    • レビュアーとして自分がふさわしくないと感じるのなら、他の人をレビュアーにアサインする。
  • コードの変更で良いところがあったら、それを作者に伝える。
Navigating a CL in Review どう進めるか
  • ステップ1: 説明をよく読む
    • プルリクの説明はよく読む。
    • 変更が行われるべきでない場合は、すみやかに、理由・代替案を丁寧に説明する。
    • 望ましくない変更が多く発生する場合は、コードレビューの前の開発プロセスを見直す。
  • ステップ2: メインとなる部分を見つけ、設計を確認する
    • まず、メインとなる部分を探す。
    • コードの変更が大きい場合は、分割を依頼する。
    • 設計に問題がある場合は、すみやかにコメントを送る。
  • ステップ3: 他の部分を読む
Speed of Code Reviews スピード
  • 集中しているときでなければ、レビューがきたらすぐに行う。レビューで自分の集中を妨げない。
  • コーディングが完了したとき、昼食の後、キッチンから戻ってきたときにレビューを行う。
  • 1営業日中までにレスポンスする。
  • レビューは翌朝の最初に行う。
  • 忙しい時は、いつできるかをレスポンスする。他のレビュアーを提案する。最初に大雑把なコメントをする。
  • 未解決なコメントがあったとしても、細かい変更であったり、後で変更されるだろうと言う場合は、承認する。
How to Write Code Review Comments コメントの書き方
  • 礼儀、あいまいではなくはっきりとした態度、尊敬する気持ちをもつ
  • 開発者に対してコメントをしない。コードについてコメントをする。
  • 「なぜか」を説明する。
  • 問題を指摘することと、具体的なコードを提供することの間で適切なバランスを取る
Handling Pushback in Code Reviews
  • 対立がおきたとき:
    • 立ち止まり、開発者の意見が正しいのではと考える。
    • 理解しようとする姿勢が伝わるようにする。
    • なぜ、提案が正しいと思うのかを、丁寧に説明する。
  • 後でやる、が発生したとき:
    • 「後でやる」が実行されることは少ないので、今すぐやるべきと主張する
    • すぐできないときは、チケットを切る
  • レビュー基準のブレにより不満が発生したとき:
    • 恐らくその不満は、レビューのスピードをあげることで解消できるので、それを検討する

プルリクの作成者向け

Writing Good CL Descriptions
  • 1行目:短い要約。コードの変更によって具体的に何がなされたのか。(英語の場合は、命令形を使う)
  • 残りの本文は、十分な説明を書く。何を解決したか。なぜそれが良い解決方法であるのか。欠点は何か。なぜ変更したか。
  • プルリクを送信する前に、見直す。
Small CLs
  • コードの変更は、1つのことだけを扱う。
  • 100行は妥当、1000行は大きすぎる。1ファイルの200行は問題ないかもしれないが、50ファイルにわたる200行は大きすぎる。
  • 分割した場合は、他のプルリクがあることを伝える。
  • リファクタリングは分離する。
  • 関連するテストコードは分離しない。
  • 大きなコード変更をする前に、先にリファクタリングのみ行う。
  • コードの変更が大きくなった時は、レビュアーに了承を得る。
How to Handle Reviewer Comments
  • コメントに怒りで返事をしてはいけない。
  • レビュアーのコメントが建設的でないときは、対面かビデオ会議する。
  • レビュアーがコードを理解できなかった場合、コードを適切に変更するか、コードにコメントを残す。
  • コメントが来たとき、反射的に考えるのではなく、立ち止まり、そのコメントは正しいか?をよく考える。
  • 対立したら:
    • コードレビューの基準を読む。

SET and REMOVE in an UpdateExpression DynamoDB

DynamoDB の put-item にて、一部の属性の値のみを更新しつつ、とある属性を削除したかったため、動作を確認しました。

An update expression consists of one or more clauses. Each clause begins with a SET, REMOVE, ADD, or DELETE keyword. You can include any of these clauses in an update expression, in any order. However, each action keyword can appear only once.

ドキュメントにもある通り、put-item では、SET, REMOVE, ADD, DELETE のアクションを指定でき、それらを複数指定できます。これらのアクションは、put-item内で1度のみしか 使えないようです。

事前準備

テーブル TableName: Members, Key: id を作成します。

aws dynamodb create-table \
    --table-name Members \
    --attribute-definitions \
        AttributeName=id,AttributeType=S \
    --key-schema AttributeName=id,KeyType=HASH \
    --billing-mode PAY_PER_REQUEST 

データを2件投入します。

  • id: "1001", name: "takada", group_id: "100", location: "Tokyo"
  • id: "1002", name: "ishikawa", group_id: "200", location: "Hokkaido"
aws dynamodb put-item \
    --table-name Members  \
        --item \
    "{\"id\": {\"S\": \"1001\"}, \"name\": {\"S\": \"takada\"}, \"group_id\": {\"S\": \"100\"}, \"location\": {\"S\": \"Tokyo\"}}"
aws dynamodb put-item \
    --table-name Members  \
        --item \
    "{\"id\": {\"S\": \"1002\"}, \"name\": {\"S\": \"ishikawa\"}, \"group_id\": {\"S\": \"200\"}, \"location\": {\"S\": \"Hokkaido\"}}"

SET のみ

まずは SET のみを使い、一部の属性のみ更新できるかを確認します。

id:1001 の location を、Tokyo から Chiba に変更します。location は、DynamoDB の予約語なので、--expression-attribute-names#locationを定義します。また、--return-values ALL_NEWで更新後の結果を返すように指定します。

結果を見ると、location 以外の値は変更されていないことを確認できます。

aws dynamodb update-item \
    --table-name Members \
    --key "{ \"id\": { \"S\":\"1001\" } }" \
    --expression-attribute-names "{ \"#location\": \"location\" }" \
    --update-expression "SET #location = :location_value" \
    --expression-attribute-values \
        " { \":location_value\": { \"S\": \"Chiba\" } }" \
    --return-values ALL_NEW
{
    "Attributes": {
        "location": {
            "S": "Chiba"
        },
        "id": {
            "S": "1001"
        },
        "name": {
            "S": "takada"
        },
        "group_id": {
            "S": "100"
        }
    }
}

SET と REMOVE

locationを変更しつつ、group_idを削除する put-item のオペレーションを実行してみます。ちなみに、group_idが削除されている状態のときに実行しても問題なく成功します。

update-expression はこのように指定します: --update-expression "SET #location = :location_value REMOVE group_id"REMOVEの前に,は不要です。

aws dynamodb update-item \
    --table-name Members \
    --key "{ \"id\": { \"S\":\"1001\" } }" \
    --expression-attribute-names "{ \"#location\": \"location\" }" \
    --update-expression "SET #location = :location_value REMOVE group_id" \
    --expression-attribute-values \
        " { \":location_value\": { \"S\": \"Kyoto\" } }" \
    --return-values ALL_NEW
{
    "Attributes": {
        "id": {
            "S": "1001"
        },
        "name": {
            "S": "takada"
        },
        "location": {
            "S": "Kyoto"
        }
    }
}
お試し

ここでお試しとして、同じ属性を SET と REMOVE で指定してみます。このように: --update-expression "SET #location = :location_value REMOVE #location"。実際に使うことはないと思われます。

結果は、以下のようにエラーが発生します。各アクションで同じ属性(document paths)は指定できないということですね。

aws dynamodb update-item \
    --table-name Members \
    --key "{ \"id\": { \"S\":\"1001\" } }" \
    --expression-attribute-names "{ \"#location\": \"location\" }" \
    --update-expression "SET #location = :location_value REMOVE #location" \
    --expression-attribute-values \
        " { \":location_value\": { \"S\": \"Kagawa\" } }" \
    --return-values ALL_NEW
An error occurred (ValidationException) when calling the UpdateItem operation: Invalid UpdateExpression: Two document paths overlap with each other; must remove or rewrite one of these paths; path one: [location], path two: [location]

さらにお試しとして、SET -> REMOVE -> SET を指定してみます。このように: --update-expression "SET #location = :location_value REMOVE group_id SET #name = :name_value"。ちなみにnameも DynamoDB の予約語です。こちらも実際に使うことはないと思われます。

SET が複数指定されているので、ドキュメントにも記載されているようにエラーが発生します。

aws dynamodb update-item \
    --table-name Members \
    --key "{ \"id\": { \"S\":\"1001\" } }" \
    --expression-attribute-names "{ \"#location\": \"location\", \"#name\": \"name\" }" \
    --update-expression "SET #location = :location_value REMOVE group_id SET #name = :name_value" \
    --expression-attribute-values \
        " { \":location_value\": { \"S\": \"Kagawa\" }, \":name_value\": { \"S\": \"Doraemon\" } }" \
    --return-values ALL_NEW
An error occurred (ValidationException) when calling the UpdateItem operation: Invalid UpdateExpression: The "SET" section can only be used once in an update expression;