これはなに？

Webエンジニアとして働き始めてから3ヶ月ほど経ちます。
幸いにも、この短期間でいくつか仕事を振っていただきました。
この記事は、プルリクエスト(PR)の中で指摘された内容に関してまとめ、今後の自己レビューに役立てるものです。
コメントや経験を元に今後も記事内容を更新していく予定です

非技術的視点

この項目は、非技術的観点での指摘をまとめます。
これは意識ひとつで改善できることなので、今後同様の指摘を受けないようにしていきます。

PRコメントは記載したか？

PRの概要欄を記載し忘れました。
社内のPRコメントのフォーマットを活用し、きちんと書きます。

変更ファイルは想定範囲内か？

GitHubのPRページを見ると Files changedというタブがあります。
このタブを開くと、今回のPRで変更されたファイルを見ることができます。
これらを確認して、自分が想定する範囲外のファイル変更が生じていないかを確認します。

VScodeの「ソース管理」タブを使用することでコミット前の差分を確認することができます。

lock系のファイル変更は注意

弊社は各個人のローカル上で開発を進めています。
ローカルで環境構築をする際に、lock系ファイルに変更が生じるとチーム間での環境に差が生じてしまいます。
lock系のファイルに変更点がある場合はそれが妥当なものかどうかを確認しましょう。

不要なファイルをpushしない

環境構築や動作確認で不要なファイルが生成されてしまう場合があります。
それらのうち、GitHubで管理する必要ないものに関してはpushしないようにしましょう。
具体的にはファイルを削除するか、.gitignoreに記載します。
(ただし、.gitignoreに記載するとその変更差分が生じるので注意)

そもそも想定範囲外のファイル変更をcommitしないようにする

そもそも想定範囲外のファイル変更をgitにコミットしないようにしましょう。
変更点に対して脳死で

$ git add . # すべての変更ファイルをステージング
$ git commit -m "コミットメッセージ"
$ git push

するのではなく、

$ git add <<ファイル名>>
$ git status
$ git commit -m "コミットメッセージ"
$ git push

するようにしましょう。

VScodeの「ソース管理」タブで変更が生じたファイルの差分を確認することもできます。

技術的視点

この項目は技術的観点での指摘内容をまとめます。
改善に時間がかかるかもしれませんが、最初の伸びしろとも言えそうです。

命名は適切か？

命名はコードリーディングにも関わるので、重要度が高いです。

変数は小文字、定数は大文字

一般的に変数は小文字、定数は大文字を使って表します。

例えば変数は
count
username
isAvailable または is_available
のように表し、2語以上から成る変数の場合はキャメルケースやスネークケースで記載します。

また、定数は
MAX_RETRY_COUNT
DB_CONNECTION_STRING
SERVER_PORT
のように、大文字のスネークケースで記載することで定数であることを明示的に表現することができます。

定数、変数、関数は適切な品詞から始まっているか？

定数、変数、関数のそれぞれがどの品詞から始まると分かりやすい名前なのかをChatGPTに聞いてみました。

定数: 定数は通常、名詞または名詞句を用います。定数は値が一定であり、オブジェクトや状態を表すために使われるからです。例えば、MAX_LOGIN_ATTEMPTSやAPI_KEYなど。
変数: 変数も通常、名詞または名詞句を用います。変数はオブジェクトあるいはオブジェクトの状態を格納し、その状態を表すものだからです。例えば、user, account_balanceなど。
関数: これらは通常、動詞から始まる名前を用います。なぜなら、関数やメソッドは状態の変化やアクションを表すためのものだからです。名前はその動作を明確に示さなければならないので、動詞を使います。例えば、calculate_total, set_passwordなど。

スコープの大きさと命名の関係

ChatGPTと会話をする中で、名前の長さはスコープの大きさに比例すべきという手法が出力されました。
その意図を深掘りすると納得できるものがあったのでまとめます。

名前の長さはスコープの大きさに比例すべきという原則は、その名前が使われる文脈やスコープ内で考慮すべきです。
変数やメソッドのスコープが大（グローバルな変数やクラスメソッドなど）の場合、その名前はそれが何を意味し、どのように使われるべきかを明確に伝えるべきです。
そのため、より詳細で説得力のある名前（つまり長い名前）が必要になることがあります。
一方、スコープが小さいもの（例えば、メソッド内のローカル変数）は、その使用範囲が制限されるため、より短くシンプルな名前でも十分に意味を伝えることができます。
また、親子関係のスコープの場合も、親のスコープの名前が長く詳細であれば、それに連結する子のスコープの名前は相対的に短くても理解しやすいです。
ですので、名前の長さは絶対的な基準に基づくべきでなく、その名前が使われるスコープと文脈に基づいて適切に選ばれるべきです。

確かに、子スコープ内で定義される変数名は親スコープありきのため短くできそうです。
また、for文内ではiやvalueといった短い名前を一時的に使用することもあります。

初見でコードの処理が理解できるか？

コードは書く時間よりも読む時間の方が長いという名言がある通り、読みやすさが重要です。

大きなスコープを持つクラス内の処理をメソッド化する

既存のコードを読んでいると、大きなスコープを持つクラス(メソッド)には細かい処理を書かず、メソッドや変数で構成されていることに気がづきました。
これは、そのクラスを見たときに一発で処理内容を理解するためです。
例えば、「チャーハンを作る」というメソッドについて考えてみます。

def チャーハンを作る
  材料 = 材料を準備する
  ご飯を炒める
  材料を混ぜる(材料)
  皿に盛り付ける
end

のようにすることで、内部のメソッドがどのような処理をしているか理解していなくても、チャーハンを作るというメソッドの処理内容を予測することができます。

ただしこれは

大きなスコープをもつクラスの命名が適切であること
メソッドの命名が適切であること

という条件が必須となります。

DRYの原則に則っているか？

コード内で同様の処理がある場合は、共通化することで可読性が増します。
DRYの原則の意義に関しては多くの方が言及しているので、今更僕が何かを言う必要はないと思います。

が、それでもDRYの原則の重要性が叫ばれているということは、これが実現していないからだと伺えます。

初心者エンジニアの私としては、
「繰り返し処理がおこなわれている箇所は発見したが、どのように共通化すれば良いのかが難しい」
つまり、共通化する技術力が無いことが問題だと考えました。

そこで、ChatGPTに以下のように指示をしてコードを提案してもらいました。

prompt

以下の2つの処理をDRY原則を実現するように共通化したメソッドを作成してください。

<<1つめの処理>>

<<2つめの処理>>

もちろん毎回ChatGPTに任せるのではなく、

ChatGPTに任せる
共通化メソッドを自分で作ってみて、ChatGPTにリファクタリングしてもらう
自信をもって自分で共通化メソッドを作る

といったように、段階的なレベルアップを目指したいです。

ファイルを過度に分割してないか？

例えばJavaScript系のプロジェクトにおいて

main.js
- 実行するメソッドを集約したファイル
config.js
- 設定ファイル
utils.js
- 呼び出すメソッドを集約したファイル

のように過度なファイル分割をおこなうことはコードの可読性を損なうことになります。
この場合はmain.js, config.jsだけで十分だと気づきました。

依存度が高くないか？

複数のソースファイル間でデータのやりとりをする場合、関連するデータと機能をグループ化することでコードの理解と管理が容易になります。
しかし、あまりに密接な交互作用は問題を引き起こすことがあります。
一つのファイルが他の多くのファイルに強く依存していると、そのファイルは変更が困難になり、疎結合の原則に反します。

具体的には、関数内で他のファイルで定義した定数を使用するとき、直接その定数を参照するのではなく引数として渡す方が適切です。

悪い例

CONSTANT = 10
def my_function(x)
  return x * CONSTANT
end

この例では、CONSTANTという定数を関数外で定義し、その定数を関数の処理内で呼び出しています。
このとき関数はCONSTANTの値に対する高い依存性を持ちます。
仮にCONSTANTの値が変更された場合に、その変更は関数の結果に直接影響を及ぼします。
また、この関数はCONSTANTの定義が可用であるコンテキストでのみ使用可能であるため、その再利用性が制限されます。

このような処理は、以下のように改善することができます。

改善例

def my_function(x, constant)
  return x * constant
end
my_function(5, 10)

constantを引数で渡すことで、関数は外部の定数に依存せずに動作することが可能になり、再利用性が大幅に向上します。
CONSTANTのような特定の値に依存せず、関数は任意の定数値で動作できるようになります。
関数の動作が引数の値に基づくため、異なるコンテキストでの利用やテストが容易になります。

依存度を小さくすることは、以下のようなメリットが考えられます。

再利用性: 関数が外部の定数に依存しない場合、その関数は他のコンテキストでも再利用しやすくなります。その関数は今後、異なる定数値を引数として受け取ることが可能です。
テストしやすさ: 関数が外部の定数に依存しない場合、その関数は独立してテストしやすくなります。テストケースを作成する際に、任意の定数値を引数として提供できます。
明確な依存関係: 関数が利用するすべてのデータが引数として明示的に渡されると、その関数の依存関係がはっきりします。これは、関数がどのデータに依存しているか理解する上で非常に重要です。

以上の理由から、関数内で他のファイルで定義した定数を使用する場合は、それを直接参照するのではなく、引数として渡すことが推奨されます。

おわりに

自己レビュー観点をまとめ、いつでも参照できるようにしました。
今一番理解したい本を置いておきます。