やる気がストロングZERO

やる気のストロングスタイル

読み解きづらいコード

自分が「読み解きづらいな」と思ったコードに対して、なぜ読み解きづらいと思ったのかを考察し、
また、そうならないようにどうしたいかを考えたので書いてみる。

個人的な考えだし、エンジニアレベルとか状況とかで変わると思うし、僕が見えてない事情とかもあると思うけど、一旦そういうのは無視して書いてる。

フックベースで処理が組まれていて、フックの種類が多くネストが深い

イベント駆動みたいに「なんらかをフックして次の処理が実行される」みたいな機能をふんだんに使って処理を組まれていると、次なにが実行されるのかが全然わからない。

たとえばActive Recordは以下のコールバックがある。

before_validation
after_validation
before_save
around_save
before_create
around_create
after_create
after_save
after_commit/after_rollback

before_validation
after_validation
before_save
around_save
before_update
around_update
after_update
after_save
after_commit/after_rollback

before_destroy
around_destroy
after_destroy
after_commit/after_rollback

これらを全て確認しないと処理を追えない。
実行順序も意識しておく必要もある。

さらにそのコールバック処理の中で別のActive Recordが使われていたりすると、そのコールバックも把握して追っていかないといけない。。

これらのコールバックを駆使して組まれた処理は

someObject.save()って実行すると上手く処理されるけど、どういう処理が行われているのかよくわからん。処理を追うのも大変。。」

という感じになる。

対策:
フックに過剰に頼らない設計にする。(フックに向いている処理ってのはあると思う)

何が実行されるのかを外部が制御してる

「実行メソッド名が外部から渡ってきてて、そのメソッドを実行する」みたいなやつ。
実際に外部からデータもらわないと何が実行されるかわからん。

大体は「内部処理を外からも汎用的に使えるようにした」みたいなやつがこういう感じになってる気がする。

対策:
外部が内部の実行メソッド名を知っているという状態がそもそもおかしい。
何を実行するのかは内部で制御するようにする。

メソッドだけ見ても何をしているのかよくわからない

こんな感じのやつ。

calculator = New Calculator(user)
calculator.execute()
price = calculator.price

priceがどういうデータからどういう計算をされた値なのか全然わからん。
ユーザー情報を渡してなんらかの計算をしているっぽい雰囲気しかわからん。
executeメソッドの中身を見ると、データベースから色々データを取得して、なんらかの計算をするロジックが長々と書かれていたりする。

対策:
引数と返り値を使って、なんのデータがどういうデータになるのか解るように表現する。
不必要にに広範囲のデータを渡さないようにする。

単体で動かせない

コードリーディングで挙動を読み解けない場合、実行してみて動きを見たいが依存が多く動かせない。
巨大なダミーデータの作成が必要になって、どういうデータが必要なのかを理解するためにコードリーディングが必要になって、読み解けなくて、、

対策:
依存をできるだけ減らしていつでもどこでも単体で動かせるようにする。

実現したいことに対して実現するための構成要素がなんか多い

1つのことを実現するために5個くらいclassが存在したり、5個くらいテーブルがあって絡み合ってる。 その思想を読み解けず、なんのためにこれだけの要素があるのかわからないので処理が追いにくい。

対策:
それを実現するのに一番自然に思い浮かぶロジックをそのまま表現できるようなコードにする。
今必要ではない念の為の機能のために不要にコードを太らせないようにする。(使われてない機能だから余計読み解きにくい。。)