はじめに
きっかけは後輩のコードレビューをしているときです。後輩は私がその当時、知らないメソッドを使っていました(flatMap
というやつ)(てか後輩私より優秀、、、)
具体的には以下のようなコードを後輩は書いていました。
const roles: string[] = [
...groupMembership,
...realmRoles,
...clientRoles,
].flatMap((fullName) =>
fullName.split("/").filter((name) => name.startsWith(rolePrefix))
);
const clientRoles: string[]
= Object.values(loginInfo.resourceAccess ?? {}).flatMap((client: any) => client.roles ?? []);
この時自分は「やばい、自分のコーディング力がなさ過ぎて読めなさすぎる、、」
「ひとまず、こういった自分が知らないメソッドは、自分の勉強不足として覚えて(調べて)いくのか、他のメジャーなメソッドで書きかえるよう指示するべきか、どうしたらいいんだろう。。。」
なので、別部署にいるつよつよエンジニアに上記の内容を質問してみました。
回答
以下要約になりますが、回答です!
- 知らなくて当然、調べて当然のスタンスでいった方がいい!
- そもそも頻繁に作成・更新され、使用言語も案件や時流で変わる業界で、全て把握している人なんていない
- 使い方次第でコードが自然な英語に近づいて可読性を高めるので、ガンガン使った方が良いと思います!!
確かに回答の通りで、知らないことの方が多い業界なので、コードレビューしながら知らないことは自分も学習していくスタンスで行こうと思いました。
後は以下のような注意点や補足を教えてくれました。
注意点
- 脆弱性が指摘されていたり利便性の上がった代替メソッドが既にあり非推奨になっているメソッドは、代替手段に切り替える
- 新しすぎるメソッド(例えば ES2023 で追加されたもの)はトランスパイラが対応していない
- そのメソッドを使うことで処理の説明になっていない場合は、別の手段を考える
実際にさっきのコードで言うと、以下のような指摘をされていました。
つよつよ「flatMap()
は配列に対して flat()
と map()
という別の処理を一度に行いますので、コードを読む人の脳みそのリソースを結構消費します。
以下の処理では client.roles
の有無によるフィルタリングだけを目的としているので、flatMap()
より、シンプルに filter()
を使った方が理解しやすいです!」
修正前
const clientRoles: string[]
= Object.values(loginInfo.resourceAccess ?? {}).flatMap((client: any) => client.roles ?? []);
修正後
const clientRoles: string[] = Object.values(loginInfo.resourceAccess ?? {})
.filter((client: any) => client.roles)
.map((client: any) => client.roles)
.flat();
補足
- VSCode を使っていてなおかつ適切に型を定義していれば、マウスホバーでメソッドの説明が表示されるので、知らないメソッドをあえて避ける必要はない。
- javascript で比較的新しめの機能はこの辺を見ると、キャッチアップ出来るとのこと。
さいごに
つよつよエンジニアの助言・視点を基にこれからコードレビューしていきたいと思います。
また、ちゃんとわからないからってあきらめずに勉強していく姿勢は忘れないようにしたいですね。。