コードレビューの時に気にしている、べからずTierリスト

こんにちは！アルダグラムのKANNAの開発お手伝いをさせて頂いているoubakiouです。

KANNAでは主にバックエンドにRails+graphql-rubyやKotlin+DGS、WebフロントエンドにTypeScriptとReactを採用していて、私が参加するチームでの仕事もそれらを触る事が多いのですが今回はそこでコードレビューをする際に気にしている「べからず」をティア別に見ていきましょう。

特に理由なくlintを無視してはいけない

アルダグラムでは利用エディタの規定や制限はありませんが、Webフロントエンド開発で一番利用者が多いのはVSCodeでextensions.jsonにlint表示等のために必要な拡張プラグインリストが整備され半自動でインストールされるようになっています。VimなどVSCode以外のエディタを利用する場合には同等のリアルタイムlint表示ができるよう自主整備する事が推奨されています。

Webフロントエンドの .eslintrc では eslint:recommended や jsx-a11y/recommended などの一般的なものに加え、GraphQLファイルには@graphql-eslint/operations-recommendedなどもかかるようになっています。

lintルールで表現できている「べからず」はそれが無視されない限りはこのTier表に含める必要もあまり無くなるためこれはSティアにしたい所なんですが

Rubyバックエンドで利用しているRuboCopはオピニオネイテッドで大胆なauto-correctをしようとしてきて、コードを壊さずにそんな変更を自動でやれるんだーと感心していると普通にコードを壊してくる事がごく稀にあるのでAティアです

またKotlinバックエンドにおいてはIntelliJプラグインから使うdetektにインデントのauto-correctで循環無限怒られを食らった事があるのでやっぱりBティアです

補足

lintルールによってCIで止まる止まらないがあります。後から導入されたルールなどで既存コードでの違反箇所が多い場合にCIでは止めない過渡期運用をしているものもあります。またRuboCopの名誉のために付け加えるとauto-correctでコードが壊れたのは私の人生において3回ぐらいでここ数年は発生していません

巨大すぎるもの(CCの高すぎるクラスや関数、巨大UIコンポーネント)を作ってはいけない

巨大なものは単純に読みにくい・テストが書きにくい、という理由だけではなく機能として分割に意味がある場合もあります

例えばReactにおけるコンポーネントという単位は再レンダリングやメモ化の単位であったりRSC(React Server Components)境界の単位であったりもします

小さければ小さいほど必ずしも良いという話ではありませんが、あまりに巨大すぎると軟弱なプログラマの脳内メモリに乗り切らず、SWAP!SWAP!と言いながらくるくると回って動作を停止してしまうのでSティアです

補足

RuboCopでは巨大さ・複雑さの指標としてCC(Cyclomatic complexity)ではなくABC sizeを利用しています

特に理由なくMutableなオブジェクトを作ったり再代入可能変数を使ってはいけない

特に理由がなければNSMutableStringよりもNSStringのようなimmutableなクラスや、sortよりもtoSorted(Baseline 2023)のような非破壊的メソッドを利用し、自作する場合でも特に理由がなければimmutableなクラスとして作り、もしmutableなクラスを作る場合にはうんざりするような長ったらしい名前をつけておきましょう

またJavaScriptであれば理由がない限りは再代入可能なletではなくconstを利用します

コードを読む際に、そのオブジェクトや変数の状態変化を短期記憶したり意識したりするギミックが減るので脳内メモリ消費量を抑えられ、僕みたいな枯死しかけた脳細胞でも読みやすいコードになるんですけど、ほとんどがimmutableなクラスでも成り立つような性質のアプリケーションと環境でのコードを書いている場合に成立する話なのと、複雑度の低い関数だったり小さいスコープの中であれば実害があまりないので、これはBティアにしときましょう

特に理由なくany型を使ってはいけない

TypeScriptを使っていたら何度となく耳にする話題だと思うんで詳しくは触れないんですが、僕の好きな24時間フィットネスジムと名前が似ているのでこれはSティアです

補足

Web版KANNAのFEにおいてはany型を少なくとも明示的に使っている箇所は0のはず(サービス立ち上げ時は当時のNext.jsのデフォルト設定のまましばらくstrict:falseで開発されていたがその後true化対応された)で、unknown型を使っている箇所がごく一部にあります

特に理由なく型アサーション(as Hogeやhoge!)を使ってはいけない

これもany型と同じでそのリポジトリの型信用度に関わってくるので大事なんですが、僕の心の中のYouTuberが喋るのに疲れてきちゃったのでBティアです

ところでRDBの世界ではNULL値の伝播(NULLs propagate)という考え方がありますが、プログラムの世界におけるnullability(NULL可能性)も伝播します。nullabilityを型で表現できる環境においては手動型アサーションのような人間の注意力に強く依存した介入やForced Unwrappingのような危険な操作を無駄にやらずに済むよう、日頃から適切にnullabilityを隔離・局所化しておくのも良い習慣でしょう。

特に理由なくuseEffectを使ってはいけない

https://ja.react.dev/learn/you-might-not-need-an-effect

ReactのuseEffectは原始の力に満ち溢れていて強力で最高にエキサイティングで、本来useEffectを使わなくてもいいコードであってもuseEffectを使って連鎖反応爆弾に仕上げる事ができます

もしあなたがActive RecordコールバックやRDBトリガーの乱射事件に巻き込まれた事がある被害者ならuseEffectが濫用された場合のメンテナンスの難しさについても想像する事ができるでしょう

ただもちろん正当な理由があってuseEffectを使う場面も割とあるのでCティアです

before-you-use-context

https://ja.react.dev/learn/passing-data-deeply-with-context#before-you-use-context

ReactのContext機能は幾重にも重なったView階層をぶち抜く圧倒的なパワーをアナボリックステロイドのようにあなたへ与えてくれます

もちろんDティアです

特に理由なく実装の継承を使ってはいけない

ある時点では継承が妥当なアプリケーションコードであっても、3年後の仕様でもそうである事を保証できる人はいません

そうなった時に変更はどれぐらい容易なのか、あの伝説的なフィットネス・トレーナー Effective Javaが言うようにコンポジションやデリゲーションではだめなのか

実装継承が存在する言語を使っている場合には継承を使う前にリスクとリターンが見合っているのか、子供と自分との自他境界が曖昧でカプセル化が壊れたtoxicな接し方になっていないか深呼吸をしてみましょう

親は親、子は子であってそれぞれが独立した個人です

Aティアです

という所で今回の動画は終わろうと思います

もしこの動画が面白いと思ってもらえたら、ライクとチャンネル登録の方もぜひお願いします

次の動画でまたお会いしましょう

See you next time.

もっとアルダグラムエンジニア組織を知りたい人、ぜひ下記の情報をチェックしてみてください！