skillup

技術ブログ

プログラミング全般 プロジェクト管理

コードレビュー時のポイント

投稿日:2021年12月5日 更新日:

コードレビュー(仕様的な点ではなくて規約的なところのチェック)などで気をつけることなど。

ポイントとしては検知ツールで確認するコストを減らすことが一番大事(カスタマイズの方法について調べておく)、あるいはコミット時に静的解析ツールが走って、ルールを守らないとコミットできないなど。

人チェックをしないのがコツだが、する場合、10以上のルールを同時にチェックすると漏れるため、メソッド単体ぐらいに切ってゆっくり見ていくほうが良いでしょう。普段からチェックし、チェックリストを作り、一気にチェックをしないことが大切だと思います。

規約に関する確認

ありがちなルール

文字のキャメル、スネークの統一、その他ルール

おそらくツールで検知できるのでこれで回避。稀にerrmsg(errMsg)のように検知できにくいものもあるので注意。

スコープの長さと変数名を対応させるなども、コードを書いていて、癖がつく部分もあるが人チェックが必要。

if文やfor文のスペースや括弧の位置

癖をつけておけば大丈夫(目が違和感を覚える)だと思うが、100%になることはなかなか難しいので、ツールでチェックできるようにしましょう。

コメントの書き方

Document作成ツールでチェックするのが一番わかりやすいが、内容のチェックまではおそらく人チェックがいると思います。

入れる場所などは完全なシステム的なチェックは難しいので人チェックで。

行の長さ、メソッドの長さ、クラスの長さチェック

普段からコードを書いて癖づけするのが一番(ここだけに限りませんが・・・)

1行あたりの場合はチェックがつくことが多い(100文字以下など)ですが、メソッド、クラスの長さなどはチェックができないことが多いでしょう。

この点は分割することを癖にするのが一番良いく、規約云々ではなく、このほうが書きやすくなることが多いです。

引数チェック

5以上を超えると見にくくなります。規約もそうですが、こちらも保守性が高いコードを書いておけば必然的に身につくはずです。

特定メソッドの推奨

一番見落としがち。例えばJavaではStringBuilderで文字を追加する、Listの定義ではArrayListやHashListを使わないなど。同時に複数チェックを入れると見逃しやすいので、この点だけに注意してチェックする。

言語自体への慣れもそうですし、普段から模範的なコードを書いておく必要があると思います。

コードの内容に関するルール

命名

  • 名称が適切か、冗長すぎないか
  • メソッドに関して規則があるか(get、setなどの頻出のものを多用しすぎていないか)

責務の分離

  • コードのクラス、メソッドの役割が適切か
  • 適切に責務に分離されていれば、Controllerでもない限り、メソッドがながくなることはないはず
  • 各クラス分けが適切か(Controller、Service、Modelなど)
  • Controller、Service、Modelの仕事がそれぞれ1つずつか(色々な仕事をさせていないか)

可読性

  • 引数が多すぎないか数は適切か
  • 1つのメソッドで1つの責務になっているか
  • 引数に必要以上に配列を使っていないか(ブラックボックスになりがちなので、少なければ直接引数を。多ければそれ用のObjectを。)
  • 定数などを定義Enumなどで置き換えているか(他人が見たときにコードがわかりやすいか)
  • 配列を使いすぎていないか(役割を持ったオブジェクトに置き換える、分けるなど)

型のチェック

  • DBに入れる場合に型チェックしたものを使っているか。setterの方がfillなどで一気に値を入れるよりも値がしっかり担保される
  • nullのある場合、可能性を常に考慮しているか
  • getter,setterなどでこれらの値の保証ができているか
  • 不十分なメソッドや条件判定(3重イコールではなく、2重イコールを使っていないか。またemptyなどを使っていないか。正しくはis_nullなど。)

抽象化

  • コードが適切に抽象化されているか(共通のものが多い場合は親を作り、親子関係ができているか、あるいは親子の関係が適切か)

保証された値を使っていないか

  • requestパラメータはバリデーションを通過したもののみを使う
  • 何らかのfilterを通過した値を使っているか

負荷

  • 大量のデータを一括で取得していないか(chunkなどで小出しにとる、分割するなど)
  • 画面の場合はデータ量を考えて設計されているか
  • レスポンス時間が考慮されているか
  • そもそも使用するミドルウェアが正しいか(DBでなくNoSQLを使うなど)

その他

  • フレームワークの標準の機能などを使っているか
  • テストコードが適切か
  • プロジェクト内のコーディングの各種ルールを守っているか

-プログラミング全般, プロジェクト管理
-

執筆者:


comment

メールアドレスが公開されることはありません。 * が付いている欄は必須項目です

関連記事

no image

スクラム開発をやってみて

アジャイルの定義に関してはエンジニアによって色々なものがあると思われますが、自分の場合一番綺麗に説明されているな・・・と感じたのは以下の記事でも紹介した書籍でした。 アジャイル開発について 現場ではた …

no image

コミュニケーション能力の定義

新卒でも既卒でも採用基準で非常に大きい要素といえばコミュニケーション能力かと思います。 が、この言葉、明確な定義がなく、私もいろんな会社でいろんな人と仕事をしてきましたが、明確な言語化がはっきりとでき …

no image

テスト項目の作り方(縦項目について)

テスト項目の作り方について。 単体テスト書のレビューをしていて、なるべく効率的に網羅的にできるテスト仕様書の作成について。 納品物としてではなく、開発の高速化と品質をあげるためのテスト仕様書を。 Co …

no image

サロゲートキーと主キーに関して

今までも何度か触れたサロゲートキーと主キーに関して。 今までの参考リンク 論理設計のグレーノウハウ サロゲートキー サロゲートキーに関して 主にシステム設計的な考察が多かったので、今回はユースケースか …

no image

PCクラッシュ時に備えて

先日、ずっとメインで使っていた会社のノートPCがクラッシュし、再起不能になりました。ファイルなんかはクラウドで管理していたものが多かったので実害はあまりなかったんですが、当然ゼロではありませんでした。 …

アーカイブ