Clean Codeを読んで改めて思ったこと、学んだこと
本を買ってから読み忘れていたので読後に思ったこと、学んだことを書いてます。
[Clean Code]
https://www.amazon.co.jp/dp/B078HYWY5X/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1
SIやってる立場として改めて思ったこと
たとえば「クラスを小さくする」ということについて、開発者(実装者)は直感的に理解できたとしても、SI現場の調整屋さんとかからすると、「小さいクラス」→「クラスが増える」→「全体の絵を見失い、小さいクラスって面倒じゃない」って、なることがあって「動けばいい、神クラス最高」ってなることが現場で多くある。というかSIの現場は大抵そんなもん。
神クラスが持っているメソッドはちょっと引数が違うだけのオーバーロードだらけのstatic
メソッドやら意味不明な名前のメソッドで溢れて、使い勝手が悪くなって、結果的に保守のコストが高くなって、トラブルあったときにスキルの高い人じゃないと解析できなくなって、結果的にスキルの高い人を外から集めて新規の案件にそういうスキルの高い人をアサインできなくなりますよ。とまで言っても「神」に固執するんだな。
上流工程だけやるSEがこんな感じだし、年次が上の人も開発(実装)してこなかったし、一緒に働く協力会社さんもこの慣習が染み付いてるから、日本のSIは微妙なんだろうな。
ってことを思いつつ、どうやったらクリーンコード、リファクタリング、世間一般のオブジェクト指向をSIで実現できるのかを常に考えてしまう今日この頃。
関数(メソッド)
当たり前のことだけど、他人に説明するときの参考になった。
関数の引数
原則
関数の引数は理想的には0
(二ラディック)、その次が1
(モナディック)、その次が2
(ダイアディック)、その次が3
(トライアディック)。これ以上は避けるべき。
避けるべき理由
- テストケースでの組み合わせを考えるコストが発生し、テストが難しくなりかねない。
- 引数の意味を解読するのに労力を使う。
- 実装の詳細を知ることになりかねない。
フラグ引数
原則
関数にフラグ引数は持たないようにする。
避けるべき理由
Boolean
のフラグ引数は関数が2つ以上のことを行っていることを示唆している。
class Foo { // 「判定する」、「出力する」の2つのことを行っている。 def render(isSuite: Boolean) = { if (isSuite) println("suite!") else println("non suite!") } }
renderForSuite
とrenderForNonSuite
に分けるようにする。
class Foo { def renderForSuite = println("suite!") def renderForNonSuite = println("non suite!") }
コマンド・照会の分離原則
関数は何らかの処理を行うか、応答を返すかのどちらかであるべき。 両方はNGという原則。たとえば以下のような関数はNG。分けて考えようというもの。
import scala.collection.mutable val config = mutable.Map.empty[String, String] // コマンド(更新)と照会を一緒にしてしまっている def set(key: String, value: String): Boolean = { config.update(key, value) if (config(key) == value) true else false }
クラス
クラスを小さくする
以下のような責務が多すぎる神クラスをつくらない。
クラス名はそのクラスの責務を表すべき。
class God { def doSomething1 = ??? def doSomething2 = ??? def doSomething3 = ??? def doSomething4 = ??? def doSomething5 = ??? def doSomething6 = ??? ... def doSomething100 = ??? }
- 名前づけがクラスのサイズ(責務)決定の最初のよりどころ
Processor
,Manager
,Super
のような曖昧な名前は、責務が不適当に集められているサイン。- 単一責任の原則(Single Responssibility Principle: SBP)を意識しよう。
- クラスの簡単な解説は、「もし」、「そして」、「あるいは」、「しかし」といった単語を使って25文字以内で。「そして」を使用している場合は、責務を持ちすぎているサイン。
デメテルの法則
直接インスタンス化したものか、引数として渡されたもの以外使ってはいけない。
たとえば、以下のクラスがあったとして、
// 氏名を表すクラス case class Name(first: String, last: String) { def fullName = s"$first $last" } // 年齢を表すクラス case class Age(value: Int) { require(value >= 0) def isAdult: Boolean = value >= 20 def isChild: Boolean = value < 20 def isSilver: Boolean = value >= 65 } // 顧客情報を表すクラス case class Customer(name: Name, age: Age)
これらのクラスを利用した以下のshowProfile
メソッドはデメテルの法則に違反している。
object Program { def showProfile(customer: Customer) = { println(s"My name is ${customer.name.fullName}.") } }
勘違いしやすいが、showProfile
メソッドをこのように書いてもデメテルの法則に違反してることに変わりはない。
def showProfile(customer: Customer) = { val name = customer.name println(s"My name is ${name.fullName}.") }
どちらにせよ、
customer
からname
を取得してfullName
で参照している。name
を直接インスタンス化してたものでもなく、引数として渡されたものでもない。
の点で違反している。
そもそも、デメテルの法則抜きにしても、customer
からname
を取得して、fullName
のプロパティを参照するのは具体的すぎる。
Customer
クラスのname
をゲッターで公開していることもよくないが、それは置いておいて(現実的にゲッターを排除することは難しい場合もあるので)、上記のケースではshowProfile
メソッドをCustomer
クラスに定義する。
case class Customer(name: Name, age: Age) { def showProfile: Unit = println(s"My name is ${fullName}.") }
これは、デメテルの法則に違反していない。
アクティブレコード
DTOの特殊形態としてアクティブレコードがあるけど、あくまでアクティブレコードはデータ構造として扱い、ビジネスロジックを持ったオブジェクトは別に用意しましょう。
// アクティブレコード class Person private (var name: String, var age: Int) { // DTOのメソッド def getName = name def getAge = age def setName(name: String) = this.name = name def setAge(age: Int) = this.age = age // アクティブレコードのメソッド def save() = ??? def find() = ??? }
Railsのアクティブレコードもこれに当てはまるなって思った。全属性をattr_accessor
で公開するとカプセル化の意味がないから、ビジネスロジックを持ったクラスは別に作成しよう。
FIRST(クリーンテストの5つの原則)
これはテストやるときに他人に説明するときの拠り所として暗記してもいいなって思った。
- 高速である(Fast)
- テストは高速でなければならない。
- テストの実行に時間がかかると、テストを頻繁に実行する気が削がれてしまう。コードをきれいにするのが億劫になってしまう。
- 結果的にコードが腐り始める
- 独立している(Independent)
- テストはお互いに関連すべきではない。あるテストが後続テストの前提じょうけんを準備してはいけない。
- すべてのテストは独立していて、好きな順序で実行できるようにしておくべき。
- テストに関連があると、あるテストの失敗が、後続テストの失敗につながる。診断を困難にする。
- 再現性がある(Repeatable)
- テストはどんな環境でも再現可能でなければならない。本番環境でもQA環境でもノートPCでも。
- 動かない環境があるということは、テストが失敗する言い訳をいつも抱えているということを意味する。
- 自己検証可能(Self-Validating)
- テストは成功か失敗かのどちらかを出力するべき。
- 長いファイルを読まないとテストの結果がわからないようなテストは避けるべき。
- テストが自己検証可能でないと、失敗判定が属人的になってしまう可能性がある。
- テストは成功か失敗かのどちらかを出力するべき。
- 適宜性がある(Timely)
- テストは必要なあときにすぐ書けなければならない。
例外
呼び出しもとが必要とする例外を定義する
サードパーティのAPIを呼び出すとき、そこで発生したサードパーティ依存の例外を翻訳してアプリケーションの例外を伝えましょうというプラクティス。
こういうのがあったら、
object MyApp extends App { def execute(): Unit = { try { // doSomething // ThirdPartyのAPIを実行するような処理 } catch { case e1: ThirdPartyAPIException01 => println(e1.getMessage) case e2: ThirdPartyAPIException02 => println(e2.getMessage) case e3: ThirdPartyAPIException03 => println(e3.getMessage) } } }
例外翻訳するためのラッパーを間に入れておき、
object ExceptionWrapper { def execute() = { try { // doSomething // ThirdPartyのAPIを実行するような処理 } catch { case e1: ThirdPartyAPIException01 => throw new MyAppException(e1) // アプリの例外に翻訳 case e2: ThirdPartyAPIException02 => throw new MyAppException(e2) // アプリの例外に翻訳 case e3: ThirdPartyAPIException03 => throw new MyAppException(e3) // アプリの例外に翻訳 } } }
サードパーティへの依存性を最小限にする。
object MyApp extends App { def execute(): Unit = { try{ ExceptionWrapper.execute() } catch { case e: MyAppException => println(e.getMessage, e) } } }
ライブラリが変わったら、ラッパを別途定義して差し替える。