こんにちは！ファインディのプロダクト開発部でエンジニアをしているham (@hamchance0215)とEND（@aiandrox）です。 この記事はFindy Advent Calendar 2024 25日目の記事ということで、2人で共同執筆しています。
この記事について
ファインディでは日頃からお世話になっている皆さんに感謝の気持ちを込めて「Findyユーザー感謝祭 2024」を12/19に開催しました。
イベント中に、参加者の一人であるミノ駆動(@MinoDriven)さんが12/25発売の著書「改訂新版 良いコード/悪いコードで学ぶ設計入門」を参加者4名にプレゼントしてくださいました！
じゃんけんで勝った4名が書籍をゲットするルールであり、hamとENDがじゃんけんの強さを発揮して書籍をゲットし、なんやかんやあってこの記事を書くことになりました。
x.comFindy感謝祭で、発売前の『改訂新版 良いコード／悪いコードで学ぶ設計入門』を配って来ました（ジャンケン大会の賞品として獲得した皆さんと一緒に） pic.twitter.com/nauFnjyCI9
— ミノ駆動 (@MinoDriven) 2024年12月19日
「改訂新版 良いコード/悪いコードで学ぶ設計入門」を使ったリファクタリングとは？
ユーザー感謝祭の懇親会でミノ駆動さんとお話しする機会があり、設計の知識を開発チームにインプットしていく方法について教えていただきました。
「改訂新版 良いコード/悪いコードで学ぶ設計入門」には、タイトルの通り「良いコード」だけではなく「悪いコード」の例もたくさん載っています。そこで、書籍に記載されている「悪いコード」をプロダクトのソースコードから探して「良いコード」にリファクタリングするという実践形式の学習をしているとのことです。
この学習方法、プロダクトのソースコードを使っていることから業務で活用できる生きた知識が身につくだけではなく、プロダクトがリファクタリングされて綺麗になっていくという一石二鳥な学習方法であり、とても興味を持ちました。
もっと知りたい方は、1/17にミノ駆動さん本人から紹介していただくイベントも企画されているのでぜひご覧ください！
実践
それでは早速プロダクトのソースコードから「悪いコード」を探して「良いコード」へリファクタリングしていきます。
今回はFindy Team+のコードを使って実践しました。
実践1: 11.4 意図がわからない名前
実践1では「11.4 意図がわからない名前」に記載されている悪いコードを参考に探しました。
「意図がわからない名前」のデメリットについて、本著の一部を抜粋しました。
あいまいな変数名では、実際の意図に翻訳する作業が読むたびに必要になります。たとえば仕様変更の依頼があったとき、対応するメソッドや変数が何であるのか頭の中で翻訳作業が必要になります。また、チームに新しく参加したメンバーに対しての説明コストが増大します。
チーム開発において、コードは書くことより読むことの方が多いため、読み手に負担がかかっている状態は好ましくありません。
次のコードは、Team+で扱っている指標の1つである「LEAD_TIME_FOR_CHANGES_HOURS(変更のリードタイム)」を管理しているハッシュマップです。今回はこちらを対象にしました。
{ statsType: 'LEAD_TIME_FOR_CHANGES_HOURS', expectData: { label: '変更のリードタイム', unit: 'h', negativePositive: true, } }
各項目を見ていくと、labelは表示名でありunitは指標の単位であることが直感的にわかりますが、negativePositiveは何を意図しているかおわかりいただけるでしょうか？
negativePositiveは「値が小さいほど良い指標か？」を表すboolean型の変数です。 ただ、negative(値が小さいほど良い) or positive(値が大きいほど良い)を示そうとしているところまでは理解できますが、trueの場合にどちらなのかを変数名だけで判断するのは困難であり、実際の設定値などから意図を推測する必要がありました。
そこで、isLowerBetterへリネームしました。 下記が修正した箇所の差分の1つです。isLowerBetterに変更したことで「値が小さいほど良い指標」の場合、ifの中に入ることがコードから読み取れるようになりました。
- if(negativePositive) { + if(isLowerBetter) { // do something }
学習しながらリファクタリングしてマージまで完了！
実践2: 7.1 ロジックの流用
Team+では、インポートに伴うさまざまな処理があり、それぞれを呼び出すBatchクラスも多く存在します。その中に、次のようなコードがありました。
module Batch class TransformXX < Batch::Base # @param [ActiveSupport::TimeWithZone] xx_after 存在する場合はXXの更新処理を行う def call(duration:, xx_after: nil) Transformer.call(duration:) # 実際にはここでXXをアップデートする処理が走る UpdateXX.call(start_at: xx_after) if xx_after rescue StandardError => e Rails.logger.error 'データ変換処理に失敗しました' end end end
問題はこの引数です。
# @param [ActiveSupport::TimeWithZone] xx_after 存在する場合はXXの更新処理を行う
これは8.6 フラグ引数の節でも紹介されているものです。特に、Batchクラスでは引数によって処理を分岐しているものがちらほらあります。
フラグ引数付きのメソッドは、何が起こるか読み手の想像を難しくさせます。何が起こるのか理解するには、メソッド内部のロジックを見に行かなければなりません。可読性が低下し、開発生産性が低下します。
このBatchクラスは管理画面から呼び出される処理で、管理画面にチェックボックスがあるので、すぐにフラグを削除することは難しそうです。幸い、この条件分岐によってどう処理を行うかはクラスに切り出されており、可読性は悪くないと思われます。
問題は、このBatchクラスが管理画面を介する処理以外のいろんな箇所から呼ばれていることです。以下は一例です。
# Before def transform_resources(start_at) Batch::TransformXX.call(duration: start_at..Time.current) end
ここでは、xx_afterが指定されていないので、xx_afterがnilになっています。つまり、実際にはTransformerの処理しか行っていません。
ということで、不要な共通化をなくして直接実行クラスを呼び出すようにしました。このリファクタリングによって例外処理がなくなりましたが、ここでは例外処理を行う必要はなかったのでスッキリしました。
# After def transform_resources(start_at) Transformer.call(duration:) end
これにより、過度な共通化がなくなり、フラグ引数が散乱する懸念も少なくなりました。
実践3: 15.1.3 条件を読みやすくする
また、次のようにunlessとnil?を使っている箇所がありました。
return unless user.nil?
二重否定となっていて読みづらかったため、次のようにリファクタリングしました。
return if user
注意するべき点としては、条件がfalseになりうる場合、nilと区別をする必要があります。そういった場合はunlessとnil?を使う方がわかりやすいかもしれません。
まとめ
今回は「改訂新版 良いコード/悪いコードで学ぶ設計入門」を参考に、プロダクトのソースコードから「悪いコード」を探してリファクタリングしてみました。
本著では、さらに大きな「悪いコード」の事例についても紹介されており、実際に探してみるとそのようなコードもちらほらありました。短い時間だったため大きな設計変更はできませんでしたが、小さな変更でもコードの可読性が向上し、コードの意図が明確になることを実感しました。
また、なんとなく「悪いコード」と感じていた箇所が、本著を読んでみるとどのような問題があるのかが理解でき、リファクタリングの方向性が見えてきたのもよかったです。
今後も、本著を参考にしながら新規の設計やリファクタリングを行っていきたいと思います。
ファインディでは一緒に会社を盛り上げてくれるメンバーを募集中です。興味を持っていただいた方はこちらのページからご応募お願いします。
