More than 5 years have passed since last update.

ソシャゲエンジニアの自分がコードレビュー時に重視する箇所33選【随時追加】

Last updated at 2019-07-03Posted at 2016-04-06

コードレビューで土日に安寧を

ソーシャルゲームは、ユーザアクセス集中と、それに伴うユーザデータ増加によって劇的に負荷が上がり、（主に土日に）サービスに影響を与えがちです。

問題があるコードは、たとえ負荷テストを行っても、作成したシナリオによっては見つけられない可能性もあります。
そういった見えない不安を払拭するという意味でも、コードレビューは重要だと思っています。

【ステキポイント】
・ソースを見ることにより、時限爆弾が土日に爆発するのを解除
・スキル共有によってメンバーがレベルアップすることにより、土日に爆発する時限爆弾の設置確率低下

まぁまとめると

これに尽きます（4歳の息子談）

今は、gitのプルリクエストという強力なレビューツールもあり、敷居がかなり低くなったのでオススメです！

チェックするポイントは5つ

コードレビューを行うにあたり、*「どんなところをチェックすればいいのか分からない」*との声が頻繁に聞こえてきますので、自分がチェックする時に心がけているポイントをまとめてみました。

重点的にチェックするポイントは、以下5点です。

実行速度が遅くならないか
データ整合性に問題がないか
運用時に問題が発生しないか
保守性が低下しないか
不正アクセスされても大丈夫か

チェック内容詳細

以降は、それぞれのカテゴリごとの、チェックする詳細内容です。
前提になっている環境はこちら。

MySQL 5.6.23
Redis 2.8.21
Django 1.8.1
Python 2.7.9

ちなみに、チェックの重要度は絵文字で表現しています。
：絶対修正してもらう
：よほどの理由がない限り修正してもらう
：ちゃんとした理由があればスルー

MySQL編

メインでチェックするのは、**「実行SQLに対してインデックスが適切に貼られているか」**ということです。
ORM普及によりSQLが見えにくくなったせいか、おざなりにしている人が多くなってきた印象です。
実行速度への影響度が大きいので重点的にチェックします。

参照：explainした時の重要ポイント

実行SQLにインデックスが使われているか？
インデックス無しでデータを取得していると、データ量と共に劇的に遅くなります。
複合インデックスが使用される場合、where全列にインデックスが使われているか？
一部の列のみしかインデックスが使われていないと、データ量と共に遅くなります。
複雑なSQLの場合、explainしてkey_lenが適切かチェックします。
整合性が重要なテーブルの場合、unique keyが貼られているか？
コードの不具合によりデータの整合性が崩れるのを阻止します。
select for updateを使用している場合、ギャップロックを考慮しているか？
存在しないデータにselect for updateをかけていると、ギャップロックによるデッドロックが発生する可能性があります。
order by列にもインデックスが効いているか？
ソートする列にインデックスが効いていない場合、filesortが発生するので遅くなります。
使われていない列にインデックスが貼られていないか？（不要なインデックスが無いか？）
不要なインデックスが多いと、insert時のインデックス再生成に時間がかかります。
カーディナリティが低い列にインデックスを貼っていないか？
カーディナリティが低い（重複データが多い）列はインデックス効率が悪いので遅くなります。
必要ない列を取得しているためにカバリングインデックスが失われていないか？
MySQLはクラスタインデックスなので、select列がインデックスでまかなわれていれば速くなります。
テーブルの想定レコード数が多すぎないか？
データ数が多いと取得速度もALTER TABLE実行速度も遅くなります。
パーティションやシャーディングなどの対策がとられているか？確認します。

Redis編

メインでチェックするのは、**「使用メモリサイズの妥当性」**です。
使用メモリが、maxmemoryを超えてしまった場合の、(maxmemory-policy設定によって)エラーになったり、eviction発生によるHIT率低下を懸念します。

expire(TTL)が設定されているか？
expireが設定されていないと、オーバーするまでメモリが肥大化していきます。
複数コマンドを実行している場合、パイプライニングを使用しているか？
複数コマンドを個別実行すると遅くなります。

ダメな例

# ループさせて１つ１つ登録していく
for key, value in data_dict.items():
    redis_connect.set(key, value)

パイプライニングを使う

with redis_connect.pipeline() as pipe:
    # ループさせてpipelineに登録しておき、一括登録する
    for key, value in data_dict.items():
        pipe.set(key, value)
    pipe.execute()

想定メモリサイズが大きすぎないか？
用意するサーバと較べて、メモリ使用量が大きすぎる場合、Hash型でまとめるなどの検討を行ってもらいます。
キャッシュとしてではなく、ストレージとして(永続化前提として)使っていないか？
基本NGですが、ソート済セット型などを使う必要があるなどのやむを得ない場合、専用のDB番号を使用してもらいます。
キャッシュとして使っているキーと同一DB番号に入っていると、緊急時にflushしにくくなるからです。
また、AOF設定（コードレビューじゃないけど）なり、生成バッチなり、データ永続化や復帰の対応方法が考えられているか確認します。

ロジック編

取得したメインデータをループさせて処理している場合、取得するのが大量データになったとしても、速度的に問題がないか？
処理部分でSQLを実行していたり、複雑な計算を行っていると、マスタデータが増えるなどの理由により、運用時に劇的に遅くなっていきます。
処理で使われるデータを、あらかじめまとめ取りできないかなど検討してもらいます。
SQL実行数が多くないか？
1つ1つのSQLは速くても、積もるとレスポンスは遅くなりますし、DBサーバの負荷にもなります。
selectの場合はまとめ取りできないか？
insertの場合はbulk insertできないか？検討してもらいます。
重要な部分はログ出力されているか？
ログがないと、ユーザからの問い合わせがあった時に調査することができなくなります。
複数ユーザが同一データに同時にアクセスしてきても大丈夫か？
例えば、フレンドの承認処理など、複数ユーザが同一レコードを更新する可能性がある場合、排他処理なり更新の順番を調整するなりしないと、デッドロックの可能性が発生します。
車輪の再作成をしていないか・DRYになっているか？
似たような処理が増えると、仕様変更が発生した時に工数増加&修正漏れによる不具合が発生します。
マジックナンバーがないか？
マジックナンバーがあると、仕様を暗記している人以外コードが読めなくなるし、値が変わると修正漏れも発生します。

ダメな例

if mission_type == 1331:
    # どんなミッションの場合の処理なのか？？

enumなどで分かりやすく

if mission_type == MissionType.PlayerLevel:
    # プレイヤーレベルのミッションだと分かる

リファクタリングされているか？
リファクタリングの中でも重視するのは、Extract Method(メソッド抽出)です。
1メソッド内に大量のコードが書かれていると、ネストも深くなりがちですし、メンバーが読んだ時にどこからどこまでが何の処理なのか分かりにくくなります。
否定boolean変数の否定分岐がないか？
結局if文内はどっちなのか？本人ですら混乱しますので、不具合を仕込む可能性が上がります。

極端な例

if not_equip != False:
    # ここはどういう時に通過するのか？？

コード編

スペルミスがないか？
いわずもがな。
メソッド名が適切か？
get_xxxと言いつつ更新もしていたりすると、メンバーがコードを読むときに混乱します。
コード規約に沿っているか？
現場により重要度は変わりますが、規約通りだと可読性がアップします。
Pythonの場合は、共通規約のpepがありますので、準拠していればOKです。
全角文字がベタ書きされていないか？
ユーザの目に触れる全角文字がコード内にベタ書きされていると、多言語対応時にコードを修正する必要が発生します。
大半のフレームワークはi18n対応方法を用意していると思いますので、そちらを使用してもらいます。
参照：Django国際化
とはいえ、別途工数がかかりますので、将来的にも国際化が必要がない場合はスルーします。

トランザクション編

トランザクション範囲は適切か？コミット・ロールバックするべき範囲で括られているか？
範囲が間違っている場合、例外が発生したらデータの整合性が保てなくなります。
DBが分かれている場合、XAトランザクションを使用しているかも要注意です。
同リクエスト内で、MySQLとRedis更新が混在する場合、例外が発生した時のことを考慮しているか？
例えば、Redisを先に更新して、DB更新時に例外が発生してもRedisはロールバックされないので、DBとの整合性が崩れます。

API編

パラメータが配列受け取りの場合、不正に数を増やされても大丈夫か？
例えばプレゼントの一括受け取りで、対象のIDを配列で受け取るような仕様の場合、配列数に上限を設けないと、悪意のあるユーザから大量データに改ざんされたアタックを受けて、サーバ負荷が上がる可能性があります。
同ユーザが同リクエストを複数回連続で送信してきても大丈夫か？
本来1回であるべきリクエストが2回連続で送信されてきた時に、排他制御が行われていないとデータの整合性が崩れます。
ありえないパラメータが送信されてきた時の対策はとられているか？
悪意あるユーザにより、パラメータを改ざんしてAPIが実行された場合、整合性チェックが行われていないとデータ整合性が崩れます。

コメント編

コメントが処理とイコールか？
コメントが間違っていたり、処理を修正した時にコメントの修正忘れがあったりすると、メンバーがコードを誤読する可能性があります。
後で対応する系のコメントには、TODO:、FIXME:キーワードが付いているか？
忘れて埋もれてしまうことが多々あるので、必ずキーワードを付けておき後でgrepできるようにします。
- TODO: ... 後で見直す場合
- FIXME: ... 後で必ず修正する場合
過去のコードがコメントアウトされて残っていないか？
メンバーがコードを読む時に雑音になりますので、基本的にはgitのヒストリに任せます。
もし、重要な経緯がありコードを残すのであれば、経緯こそが最重要ですので、経緯の説明付きで残してもらいます。

1278

1283

You get articles that match your needs
You can efficiently read back useful information
You can use dark theme

What you can do with signing up

ソシャゲエンジニアの自分がコードレビュー時に重視する箇所33選 【随時追加】