はじめに

実務では日々プルリクエストを作って、先輩からレビューをいただいています。
入社して1ヶ月ごろ、関数で書いたものを変数で書いた方がいいと言われ、修正したことがありました。
初心者でなければ、そんなの当たり前と言うような簡単な話かもしれませんが…
私と同じようなプログラミング初心者の方が、関数と変数を使い分けることで読みやすいコードを書けるように、共有したいと思います。

具体例で説明

背景

ファイルをドラック&ドロップで別のフォルダに移動する機能が実装されている。
移動したときに、どのフォルダに移動したのかわかりにくいという意見が出た。
そこで、移動先のフォルダ名を移動前にアラートで表示させることになった。

目的

フォルダの名前を取得する

設計

フォルダーの配列foldersから、移動先のfolderIdに合致するfolderを取得する
そのfolderのnameを参照する

実装

配列のfindメソッドを使って、foldersから移動先のfolderIdに合致するfolderを取得しました。
そして、取得できたfolderのnameを参照します。
findメソッドは要素が存在しない場合はundefindを返す仕様なので、?.(Optional chaining)と??（Nullish coalescing）でundefindの場合は空文字を返すように書きました。

const getFolderName = (toFolderId: number): string => {
  return folders.find((folder) => folder.id === toFolderId)?.name ?? '';
};

一見良さそうですね。
結構自信満々でプルリクエストを出したのですが…
メンターさんからレビューで「ここは関数にしなくてもいいのではないか。その場で値を取って、変数に代入すればいい。」と言われました。

特に何も考えずに関数で書いていましたが、変数に入れればいいというのです。どういうことでしょうか？🤔

関数の結果を変数で書いた方がいい例

原則、関数の中に関数を書かない方がいい
コードが短い（処理が簡単）なとき
使い回す必要がないとき

関数の中に関数を書かない方がいい

理由は、ネストが深くなってコードが読みにくくなるのを防ぐためです。
実は今回書いたgetFolderName関数は、moveFileという関数の中に書いていました。
まさに関数の中に関数を書いてネストしてしまっている状態です。
できる限り、関数の中に関数を書くのは避けましょう。

コードが短い（処理が簡単）なとき

処理が簡単なら、わざわざ関数にして長ったらしく書く必要はないということです。

使い回す必要がないとき

1回しか使わないものをわざわざ関数に書く必要はないということです。
しかし、将来的に同じ処理が他の場所で必要になることもあるため、関数にまとめておくと後で使い回しやすいというメリットもあるようなので、判断が難しいですね😲

レビュー修正

上記を踏まえて検討すると、今回は処理も一行で短く、１回しか使わないので変数として定義した方が良さそうです。
ついでに変数名も直します。

const folderName = folders.find((folder) => folder.id === nextFolderId)?.name ?? '';

確かにこっちの方がスッキリして見やすくなりました。

私なりのまとめ

学んだことを備忘録として、私なりに噛み砕いて自分の言葉でまとめてみます。

無駄なものはできる限り書かないって意識が良さそう！
関数と変数を使い分けるというよりは、変数で定義するとわかりにくいものを関数で定義するって考えた方がいいかな？
将来的に使い回すかもしれないものは関数に切り出しておくのもあり

終わりに

特に意識しないで書くと、読みにくい無駄なコードを書いてしまうことがあります。
傾向はあるかもしれませんが、確実にこうした方がいいと言う基準ではないようなので、都度根拠を考えながらコードを書いていきたいです。