JVM・ガベージコレクションに関してまとめてみた
先日、業務でフルGCが発生しアプリケーションが止まってしまうことがありました。 そもそもフルGCとは何か、わからなかったのでそこから調べて、またGCってどれくらいの頻度で発生するのか気になったので、調べて実際にちょこっと触ってみたことを書いています。
GCとは
簡単にいうと、JVM上に確保されたヒープ領域で、不要になったオブジェクトに紐づいているメモリを開放すること。
たとえば、以下のようなコードがあった場合には、Compnayのオブジェクトが100000個も作成されます。 ほっておくとメモリを圧迫してしまいます。 実際にはこんなコード書かないと思いますが、このように少しのあらゆるコードの箇所で、数多くのオブジェクトが作られメモリに乗っています。
case class Company(name: String) for(i 0 <- 100000 ) { val company = Company(s"企業-${i}") }
JVMのヒープ領域の詳細
young・oldに大別されます。 youngはさらに、eden survivorという二つの空間に分けられます。
オブジェクトは以下のようなライフサイクルになります。
object生成 -> eden -> パンパンになる -> GC（マイナーガベージコレクション） -> 開放 or survivor/oldに昇格 -> パンパンになる -> GC（マイナーガベージコレクション） -> 開放 or oldに昇格 -> oldパンパン -> GC(フルガベージコレクション)
注意点としては、どんなGCのアルゴリズムでも、マイナーGCが走るたびにアプリケーションのスレッドは全て停止されます。 なので、young領域が小さければ小さいほど頻繁にマイナーGCが走ります。(基本的には、頻繁に小さいGCが起きる方が良いらしい。)
フルGCに関しては、GCアルゴリズムによって挙動がだいぶ違ってきます。アプリケーションスレッドを停止するものもあれば、並列でフルGCを走らせるものもあります。
フルGCがやることとしては、old領域から不要なオブジェクトを見つけて、メモリを開放し、ヒープをコンパクト化する（デフラグメンテーションする）なので、長い時間がかかります。 GCのチューニングをする際は、アプリケーションによるがこのフルGCを回避する方法を見つけることが肝になってきます。
GCアルゴリズム
GCのアルゴリズムは以下の四つが存在します。 それぞれ特徴があるので、特徴をちょろっと書いていきます。
シリアル型ガベージコレクター
スループット型ガベージコレクタ
- young領域の処理に複数のスレッドを利用するので、マイナーGCは上記のシリアル型よりも非常に高速
- old領域の処理にも複数のスレッドを利用することができる
- マイナーGCとフルGCの両方で、実行時にアプリケーションスレッドを停止する必要がある
CMSガベージコレクタ（コンカレント型）
- マイナーGCに関しては、上記の二つの処理と同じ
- フルGCの場合には、アプリケーションスレッドを停止しないように、一つか複数のスレッドをバックグラウンドでold領域のメモリ解放に当てている
- バックグラウンドで動かすことになるので、CPU利用率は上がる
- 処理時にコンパクト化を行わないので、ヒープのフラグメンテーション化が発生する。
- 断片化が進行し、割り当てるメモリが存在しなくなったら、他のGCアルゴリズムと同じようにアプリケーションスレッドを停止し、単一のスレッドを使って、old領域のコンパクト化やクリーンアップを行う。
G1ガベージコレクタ（コンカレント型）
- young領域、old領域で、複数のリージョンに分割されている
- マイナーGCに関しては。他のアルゴリズム同様
- フルGCに関しては、コンカレント型なのでバックグラウンドで実行されるので、アプリケーションスレッドを停止する必要がない
- old領域も複数のリージョンに分割されているので、使われているメモリを別のリージョンに移動するという形でコンパクト化を行っている
試してみる
ここまでで、GCの基礎をインプットしたので、実際にどんな感じでGCが発生しているのかを試してみました。
sbt(1.3.8) console上で、プログラムを書いてJVMのメトリクスを取得していきます。
sbtのJVMのチューニング
sbtを実行する配下に、.jvmoptionsファイルを配置しておくと、そのファイルの中身をJVMのパラメータとして読み込まれます。
-Xms1024M -Xmx1024M -Xss1024M -XX:MaxMetaspaceSize=1024M -XX:+UseSerialGC
-Xms ・・・初期ヒープサイズ -Xmx ・・・最大ヒープサイズ -Xss ・・・スレッドスタックサイズ -XX:MaxMetaspaceSize ・・・パーマネント領域のサイズ -XX:+UseSerialGC ・・・GCアルゴリズムの指定（サンプルでは、シリアル型ガベージコレクタを指定しています）
では、実際に.jvmoptsに各項目の設定を行なって、プログラムを実行します。
jvmのオプションは、上記の設定を同じ数値にします。
~/jvm-test $ sbt
jpsコマンドで、動いているjavaプロセスを確認できるので、上記で動かしたsbtのプロセスが存在することを確認する。 sbt-launch.jarが実行しているsbtのプロセスです。
~/jvm-test$ jps 77057 sbt-launch.jar 56693 77100 Jps 3406 Main
上記のプロセスでのGCメトリクスを表示します。 jstatコマンドでの-gcオプションを使うと、該当プロセスでのGCメトリクスを取得できます。 10はメトリクスを取得する間隔です。デフォルトの単位は、msになっています。
~/jvm-test$ jstat -gc 77057 10
実際にプログラムを書いて、gcメトリクスを取得していきます。 まず、プログラムを動かす前のGCメトリクスを確認します。
➜ jvm_test jstat -gc 77057 S0C S1C S0U S1U EC EU OC OU MC MU CCSC CCSU YGC YGCT FGC FGCT GCT 34944.0 34944.0 0.0 0.0 279616.0 232912.0 699072.0 52034.1 59028.0 54605.4 7612.0 7243.9 0 0.000 3 0.157 0.157
それぞれの詳しいことは、ドキュメントに書いています。 今回の分析をしていく中で、重要な物を書いていきます。
S0C・・・Survivor領域0の現在の容量(KB) S1C・・・Survivor領域1の現在の容量(KB) S0U・・・Survivor領域0の利用率(KB) S1U・・・Survivor領域1の利用率(KB) EC・・・Eden領域の現在の容量（KB） EU・・・Eden領域の現在の容量（KB） YGC・・・若い世代のGCイベント数 FGC・・・古い世代のGCイベント数
sbtプロセスを実行した段階では、まだオブジェクトは何も作成していないので、S0U・S1U・YGCは0になっています。 立ち上げた時点で、FGCの値は既に3になっているのですが、なぜかわかりません。。。
またEUの値も0ではなく、プロセスを立ち上げた時点でEden領域のメモリは使われるのでしょうか。
以下のプログラムを動かしていきます。 1Mのオブジェクトをループで作成していきます。
for(i <- 0 to 1000) { val builder = new StringBuilder(1000000) }
1秒間隔でメトリクスを取得しています。
~/jvm-test$ jstat -gc 77057 1000 S0C S1C S0U S1U EC EU OC OU MC MU CCSC CCSU YGC YGCT FGC FGCT GCT 34944.0 34944.0 0.0 0.0 279616.0 0.0 699072.0 52811.1 99532.0 86381.2 13556.0 12524.2 72 0.183 4 0.330 0.513 34944.0 34944.0 0.0 0.0 279616.0 0.0 699072.0 52811.1 99532.0 86381.2 13556.0 12524.2 111 0.246 4 0.330 0.575 34944.0 34944.0 0.0 0.0 279616.0 17278.5 699072.0 52811.1 99532.0 86381.2 13556.0 12524.2 151 0.308 4 0.330 0.637 34944.0 34944.0 0.0 0.0 279616.0 0.0 699072.0 52811.1 99532.0 86381.2 13556.0 12524.2 190 0.371 4 0.330 0.701
EUとYGCの値が増えていることが確認できます。 上記のプログラムで作成したbuilerオブジェクトがまず、Eden領域に入り、その時点で走ったマイナーGCによって、Survivor領域には昇格せずに開放されているからです。
次に、フルGCの確認をします。 フルGCを起こしやすいように、ヒープのサイズを以下のように小さく設定しておきます。
-Xms100M -Xmx100M -Xss100M -XX:MaxMetaspaceSize=1024M
今回実行するプログラムです。
val builder1 = new StringBuilder(5000000)
初めからEden領域の容量を超えるオブジェクトを作成します。 この場合、Eden領域には入らないので、いきなりold領域にオブジェクトが移動することになります。さらに、old領域の空き容量を超えているので、 フルGCを行なってからold領域に割り当てられます。
~/jvm-test$ jstat -gc 87198 S0C S1C S0U S1U EC EU OC OU MC MU CCSC CCSU YGC YGCT FGC FGCT GCT 11264.0 11264.0 0.0 3961.1 11264.0 10713.2 68608.0 40401.1 95764.0 83780.1 13116.0 12384.1 43 0.114 3 0.166 0.279 10752.0 11264.0 10716.2 0.0 11264.0 274.5 68608.0 41973.0 98580.0 85517.2 13372.0 12405.6 48 0.131 4 0.268 0.399
一行目と二行目で、FGCの値が違うのが確認できます。
ここで疑問なのが、YGCの値が43->48・S0Uの値も変化していることです。 つまりマイナーGCが実行され、もともとEdenにあったオブジェクトがSurvivor0領域に移動しています。 Eden領域以上のオブジェクトを割り当てようとすると、まずマイナーGCが走ってそれでもEden領域に入らなければ、old領域に割り当てられるというロジックなのかなと推測しています。
まとめ
いろんな記事・ドキュメントを読んで、書いているロジック通りにはGCが動いていないように見えるところもありましたが、なんとなくGCに関して理解できました。
次回は、GCアルゴリズムでどのようにGC処理が違うのか、試したいと思います。
参考
tennis kata リファクタリング
今回の記事は、tennis-kataのリファクタリングです。
リファクタリングの記事を書いていて、かの有名なtennis-kataリファクタリングをやったことがなかったので、実際にリファクタリングをしてみました。
ただ、リファクタリングしたものを見せるのではなくて、code smellベースで解説していきます。
今回もscalaでリファクタリングしていくのですが、scala力がないためscalaっぽい書き方ではないコードが多々あるかもしれません。
もし何か間違っているなら、ご教授いただけると泣いて喜びますmm
tennis-kata
code smellが敷き詰められたコードがあるので、自分の力でリファクタリングすることができます。 scalaだけではなく、以下の画像のようにgo javascriptなど様々な言語に対応しているので、練習したい言語でリファクタリングをすることができます。
問題は TennisGame1 TennisGame2 TennisGame3と3問あり、数字が大きくなるにつれて複雑度が上がりリファクタリングが難しくなります。
この特徴として、すでにgreenになるテストが実装されているのでリファクタリングする際は、リファクタリング -> テスト実行 のサイクルを細かく回すことが大切になってきます。
あまりにもテストを実行していないと、どこまでのリファクタリングが仕様を満たしていて、どっかが満たしていないのかが分からなくなります。
重点code smellをベースにリファクタリング
では実際にリファクタリングをしていきます。
上述した通り、scalaっぽい書き方に関するリファクタリングも行います。
まず、リファクタリング前のコードを載せます。
package tennis trait TennisGame { def wonPoint(x : String ) def calculateScore() : String } class TennisGame1 (val player1Name : String, val player2Name : String) extends TennisGame { var m_score1: Int = 0 var m_score2: Int = 0 def wonPoint(playerName : String) { if (playerName == "player1") m_score1 += 1 else m_score2 += 1 } def calculateScore() : String = { var score : String = "" var tempScore=0 if (m_score1==m_score2) { score = m_score1 match { case 0 => "Love-All" case 1 => "Fifteen-All" case 2 => "Thirty-All" case _ => "Deuce" } } else if (m_score1>=4 || m_score2>=4) { val minusResult = m_score1-m_score2 if (minusResult==1) score ="Advantage player1" else if (minusResult == -1) score ="Advantage player2" else if (minusResult>=2) score = "Win for player1" else score ="Win for player2" } else { for ( i<- 1 until 3 by 1) { if (i==1) tempScore = m_score1 else { score+="-"; tempScore = m_score2;} val tempScore2 = tempScore match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } score += tempScore2 } } return score } }
臭いますねええ。
long method
TennisGame1クラスの calculateScoreメソッドがlong methodになっていそうです。
if (m_score1==m_score2) { score = m_score1 match { case 0 => "Love-All" case 1 => "Fifteen-All" case 2 => "Thirty-All" case _ => "Deuce" } }
else if (m_score1>=4 || m_score2>=4) { val minusResult = m_score1-m_score2 if (minusResult==1) score ="Advantage player1" else if (minusResult == -1) score ="Advantage player2" else if (minusResult>=2) score = "Win for player1" else score ="Win for player2" }
else { for ( i<- 1 until 3 by 1) { if (i==1) tempScore = m_score1 else { score+="-"; tempScore = m_score2;} val tempScore2 = tempScore match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } score += tempScore2 } }
これらのif分の中身を、いったんprivateメソッドに切り出しましょう。
def calculateScore() : String = { var score : String = "" if (m_score1==m_score2) { score = calculateSameTotalScore // 1. メソッド切り出し } else if (m_score1>=4 || m_score2>=4) { score = calculateAdvantageScore // 2. メソッド切り出し } else { score = calculateAdvantageOneSide // 3. メソッド切り出し } return score } private def calculateSameTotalScore = { // 1. メソッド切り出し m_score1 match { case 0 => "Love-All" case 1 => "Fifteen-All" case 2 => "Thirty-All" case _ => "Deuce" } } private def calculateAdvantageScore: String = { // 2. メソッド切り出し val minusResult = m_score1-m_score2 if (minusResult==1) "Advantage player1" else if (minusResult == -1) "Advantage player2" else if (minusResult>=2) "Win for player1" else "Win for player2" } private def calculateAdvantageOneSide: String = { // 3. メソッド切り出し var score : String = "" var tempScore=0 for ( i<- 1 until 3 by 1) { if (i==1) tempScore = m_score1 else { score+="-"; tempScore = m_score2;} val tempScore2 = tempScore match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } score += tempScore2 } score }
calculateScoreメソッドは、可読性が上がりましたね。
抽象度が揃っていない。
calculateScoreメソッドの中身の抽象度を揃えていきましょう。 具体的には、
if (m_score1==m_score2) { }else if (m_score1>=4 || m_score2>=4) { } else { }
の中身が具象になっているので、メソッド抽出をしてあげて、メソッド内の抽象度を揃えてあげます。
def calculateScore() : String = { var score : String = "" if (isSameScore) // 1. メソッド抽出 { score = calculateSameTotalScore } else if (isOneSideWonScore) // 2. メソッド抽出 { score = calculateAdvantageScore } else { score = calculateAdvantageOneSide } return score } private def isOneSideWonScore = { // 2. メソッド抽出 m_score1 >= 4 || m_score2 >= 4 } private def isSameScore = { // 1. メソッド抽出 m_score1 == m_score2 }
これで、だいぶcalculateScoreメソッドの可読性が上がりました。
ミュータブルな変数は使わない
ミュータブルな変数、var scoreが使われているので、これを無くしていきましょう。
def calculateScore() : String = { // var scoreという一時変数に入れない if (isSameScore) calculateSameTotalScore else if (isOneSideWonScore) calculateAdvantageScore else calculateAdvantageOneSide }
そもそも、scoreという変数は不要でした。
pattern matchを使う
if elseが羅列されていて可読性がないですね。
private def calculateAdvantageScore: String = { val minusResult = m_score1-m_score2 if (minusResult==1) "Advantage player1" else if (minusResult == -1) "Advantage player2" else if (minusResult>=2) "Win for player1" else "Win for player2" }
ここは、pattern matchを使ってあげて読みやすくします。
private def calculateAdvantageScore: String = { m_score1-m_score2 match { case 1 => "Advantage player1" case -1 => "Advantage player2" case minusResult @ _ if minusResult>=2 => "Win for player1" case _ => "Win for player2" } }
無駄な変数代入などがなくなり、スッキリしました。
次は、一番code smellがありそうな calculateAdvantageOneSideメソッドのリファクタリングをしていきます。
ミュータブルな一時変数を削除
だいぶ臭いそうですね。
private def calculateAdvantageOneSide: String = { var score : String = "" var tempScore=0 for ( i<- 1 until 3 by 1) { if (i==1) tempScore = m_score1 else { score+="-"; tempScore = m_score2;} val tempScore2 = tempScore match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } score += tempScore2 } score }
ここでも、ミュータブルな一時変数を使っているので、それを取り除いていきます。
private def calculateAdvantageOneSide: String = { var score : String = "" for ( i<- 1 until 3 by 1) { // pattern matchにすることで、tempScore変数(ミュータブルな変数)を取り除く val tempScore = i match { case 1 => m_score1 case _ => { score += "-"; m_score2 } } val tempScore2 = tempScore match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } score += tempScore2 } score }
処理をよく見ると、二回for文を回しており、1回目と二回目でそれぞれ処理内容がscroe1 score2と違うので、 for文は不要です。
private def calculateAdvantageOneSide: String = { val score1 = m_score1 match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } val score2 = m_score2 match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } score1 + "-" + score2 }
pattern matchの中身が重複しているので、メソッド抽出でまとめます。
private def calculateAdvantageOneSide: String = { // インラインメソッドにまとめる def calculateScore(score: Int) = { score match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } } val score1 = calculateScore(m_score1) val score2 = calculateScore(m_score2) score1 + "-" + score2 }
変数のインライン化
一回しか利用していない変数は特に変数にしなくても良いので、インライン化します。
score1とscore2変数が一回しか使われていないので、それぞれインライン化します。
private def calculateAdvantageOneSide: String = { def calculateScore(score: Int) = { score match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } } calculateScore(m_score1) + "-" + calculateScore(m_score2) }
ここまでで、ほとんどのリファクタリングできました。
で、最後に残っているのが、以下のplayerそれぞれの点数を保持するミュータブルな一時変数です。
class TennisGame1 (val player1Name : String, val player2Name : String) extends TennisGame { var m_score1: Int = 0 var m_score2: Int = 0 ....... }
いい方法が思いつかなかったので、mutable collectionのListBufferを使うようにしました。
class TennisGame1 (val player1Name : String, val player2Name : String) extends TennisGame { var m_score1: ListBuffer[Int] = ListBuffer.empty[Int] var m_score2: ListBuffer[Int] = ListBuffer.empty[Int] .... }
ここまでのコード全体を見てみます。
class TennisGame1 (val player1Name : String, val player2Name : String) extends TennisGame { val m_score1: ListBuffer[Int] = ListBuffer.empty[Int] val m_score2: ListBuffer[Int] = ListBuffer.empty[Int] def wonPoint(playerName : String) { if (playerName == "player1") m_score1 += 1 else m_score2 += 1 } def calculateScore() : String = { if (isSameScore) calculateSameTotalScore else if (isOneSideWonScore) calculateAdvantageScore else calculateAdvantageOneSide } private def isOneSideWonScore = { m_score1.sum >= 4 || m_score2.sum >= 4 } private def isSameScore = { m_score1 == m_score2 } private def calculateSameTotalScore = { m_score1.sum match { case 0 => "Love-All" case 1 => "Fifteen-All" case 2 => "Thirty-All" case _ => "Deuce" } } private def calculateAdvantageScore: String = { m_score1.sum-m_score2.sum match { case 1 => "Advantage player1" case -1 => "Advantage player2" case minusResult @ _ if minusResult>=2 => "Win for player1" case _ => "Win for player2" } } private def calculateAdvantageOneSide: String = { def calculateScore(score: Int) = { score match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } } calculateScore(m_score1.sum) + "-" + calculateScore(m_score2.sum) } }
リファクタリング前と比べて格段に可読性が上がりました。
最後に、凝集度の観点でリファクタリングします。
m_score1とm_score2は、１つのオブジェクトでまとめたいので、Score case classを作り、その中のフィールド値をしてそれぞれ定義します。
case class Score(m_score1: ListBuffer[Int], m_score2: ListBuffer[Int]) {} class TennisGame1 (val player1Name : String, val player2Name : String) extends TennisGame { val score = Score(ListBuffer.empty[Int], ListBuffer.empty[Int]) ..... }
また、privateメソッドに切り出した関数たちは、TennisGame1クラスにあるよりもScoreクラスに持たせてあげたほうが責務的に良い気がします。
自前のオブジェクトでラップしてあげることで、これまでメソッド抽出して作成したprivateメソッドを、作成したScoreクラスのメソッドとして生やすことができます。
今回のソースコードでは効果を発揮しないですが、振る舞いを持たせることで、再利用性が上がり、変更にも柔軟なアーキテクチャになります。
コード最終形態
様々なcode smellのリファクタリングをしてきました。
リファクタリング前よりは、だいぶ可読性は上がったのではないでしょうか。
package tennis import scala.collection.mutable.ListBuffer class TennisGame1 (val player1Name : String, val player2Name : String) extends TennisGame { val score = Score(ListBuffer.empty[Int], ListBuffer.empty[Int]) def wonPoint(playerName : String) { if (playerName == "player1") score.m_score1 += 1 else score.m_score2 += 1 } def calculateScore() : String = { if (score.isSameScore) score.calculateSameTotalScore else if (score.isOneSideWonScore) score.calculateAdvantageScore else score.calculateAdvantageOneSide } } case class Score(m_score1: ListBuffer[Int], m_score2: ListBuffer[Int]) { def isOneSideWonScore = { m_score1.sum >= 4 || m_score2.sum >= 4 } def isSameScore = { m_score1 == m_score2 } def calculateSameTotalScore = { m_score1.sum match { case 0 => "Love-All" case 1 => "Fifteen-All" case 2 => "Thirty-All" case _ => "Deuce" } } def calculateAdvantageScore: String = { m_score1.sum-m_score2.sum match { case 1 => "Advantage player1" case -1 => "Advantage player2" case minusResult @ _ if minusResult>=2 => "Win for player1" case _ => "Win for player2" } } def calculateAdvantageOneSide: String = { def calculateScore(score: Int) = { score match { case 0 => "Love" case 1 => "Fifteen" case 2 => "Thirty" case 3 => "Forty" } } calculateScore(m_score1.sum) + "-" + calculateScore(m_score2.sum) } }
まとめ
リファクタリングに取り掛かる前に、「このcode smellを取り除こう！！」と、code smellを意識してリファクタリングしていくと、非常にリファクタリングがやりやすくなります。
あとやはり、テストはきっちり書いておかないとリファクタリングはできないですね。
業務でもきちんとTDDデ開発をしようと改めて思いました。
最後まで読んでいただきありがとうございます。
code smell リファクタリング[lazy class編]
lazy classとは
怠け者のクラス。 クラスがほとんど振る舞いを持たずに怠けている状態のこと。（存在している意味がわからない）
なぜ悪いのか
必要がない。 冗長になる。
よくあるパターン
よくあるのは、以下のようなとりあえずserviceクラスを作って、そこに処理を投げている構造です。
package study.lazyclass class PaymentController { def payment(amount: Int) = { val service = new Service service.pay(amount) } } class PaymentService { def pay(amount: Int): Unit = { paymentRepository.save(amount) } }
payメソッドだけが、PaymentServiceクラスに定義されています。 Serviceクラスがpayメソッドと言う振る舞いしか持っていません。
ソリューション
ソリューション自体は非常に簡単です。
関数のインライン化 必要な時に作ればいい。 クラスを作るタイミングとしては、二つ以上の箇所から呼び出されたりなど、関数をまとめるクラスが必要になった時に作ればいいだけです。
class PaymentController { def payment(amount: Int) = { paymentRepository.save(amount) } }
PaymentControllerから直接、paymentRepositoryを呼び出すように変更しています。
これは、よく言われているYAGNIの法則にもそっています。
考え方
ここからがこのcode smellの重要な観点です。
lazy classリダファクタリング自体は非常に簡単にできますが、リファクタリングを実際にするのかどうかが開発手法によって変わってきます。
レイヤードアーキテクチャでアプリケーション構成を考えていくなど、最初から適応するアーキテクチャを考えているならば、上記のようなリファクタリングをする必要はないです。
関数が一つしかない場合でも、きちんとレイヤーを分けて、定義する必要があります。
例えば、レイヤードアーキテクチャを採用しているなら、下図のように依存関係は上から下の一方通行になるように設計しないといけないので、 リファクタリングのコードのような、controller(presentation層)からrepository(infrastructure層)を呼ぶことはできない。
なのである程度のcode smellを許容しながら、開発を進める必要があります。
しかし、アーキテクチャを最初から考えていくと、そのアーキテクチャのルールに乗らないといけないので、コードを書くのが窮屈になると言う理由で、
基本的に開発の序盤からアーキテクチャを考えるべきではないという開発者もいる。
なので、どんな開発手法をとるかはチームで考えて開発ルールを決定すべき。
まとめ
lazy classのリファクタリングを行いました。
最後までありがとうございます。
code smellリファクタリング[feature envy編]
第三回のリファクタリングは、feature envyです。
feature envyとは
あるモジュールの関数が、内部のモジュールよりも、外部のモジュールの関数やデータ構造とやりとりしているということ。
例えば、よくある例としては、他のモジュールのgetメソッドをチェーンして使っている場合など。
悪いコード例
例えば、以下のコードを見てください。
Houseクラスから、Personクラス Adressクラスを辿って、AdressクラスのcalcurateDistanceメソッドを呼び出しています。
明らかに、Houseクラスは他のクラスを知りすぎており、参照しすぎています。
House().getPerson().getAdress().calcurateDistance()
なぜ悪いのか
では、そもそもなぜこのような過度の外部モジュールへの参照が悪いのでしょうか？
そもそも良い設計をする時に、内部モジュールとよくやりとりし、外部モジュールとのやりとりは最小限にすることが良いとされています。
こうすることで、
処理をまとめることで凝集度が上がり、変更しても影響が少なくなる。
ソリューション
主に考えられるソリューションは以下の二つがあります。
関数の移動
フィールド値の移動
では実際に、コード例を見ていきます。
このコード例では、四つのモデルがあります。 House Price Address SalesPerson
家の値段を計算したり、家までの距離を計算したりするプログラムだと仮定します。
リファクタリング前
class Main { def main(args: Array[String]): Unit = { val address = Address(1L, 1L, 100L) val price = Price(10000) val house = House(address, price) val salesPerson = SalesPerson(1, "kin", house) salesPerson.calculatePrice } case class House(address: Address, price: Price) case class Price(number: Int) case class Address( longitude: Long, latitude: Long, price: Long ) case class SalesPerson( id: Int, name: String, house: House ) { def calculatePrice: Int = { (house.price.number * 1.1).toInt } def calculateDistance: Long = { house.address.longitude } } }
SalesPersonクラスにsmellがありそうですね。
よく見てみると、calculatePriceメソッドとcalculateDistanceメソッドの両方が、houseクラスのフィールド値を参照しています。
典型的なfeature envy smellですね。つまり、自分のフィールド値よりも外部モジュールのフィールド値をよく参照しています。
これを、リファクタリングしていきます。
この場合のリファクタリングには、フィールド値の移動・関数の移動のどちらが適しているでしょうか。
フィールド値の移動を行う、
case class House() { } case class SalesPerson(id: Int, name: String, house: House, address: Address, price: Price) { def calculatePrice: Int = { (price.number * 1.1).toInt } def calculateDistance: Long = { address.longitude } }
二つの関数内のsmellは無くなったかもしれませんが、modelの凝集度が破綻していますね。
SlaesPersonクラスが、address priceなどの関係のないデータを持つようになってしまいます。
これは、素直に関数の移動をしてあげましょう。
リファクタリング後
def main(args: Array[String]): Unit = { val address = Address(1L, 1L, 100L) val price = Price(10000) val house = House(address, price) val salesPerson = SalesPerson(1, "kin", house) // salesPerson.calculatePrice house.calculatePrice house.calculateDistance } case class House(address: Address, price: Price) { def calculatePrice: Int = { (price.number * 1.1).toInt } def calculateDistance: Long = { address.longitude } } case class SalesPerson( id: Int, name: String, house: House )
二つの関数をHouseクラスに移動させることで、feature envy smellは消え、さらにHouseクラスの振る舞いも増えて凝集度が高まりました。
まとめ
僕の感覚として、 隣のさらにまた隣のモジュールのデータにまでアクセスしようとしているなら、それはfeature envyだと疑った方がいいかと思います。
上記のリファクタリングのように、関数を定義する場所が違うのか、それとも、オブジェクトのフィールド値を移動させた方がいいかと。
最後までありがとうございました。
次回は、lazy classに関しての記事を書きます。
code smellリファクタリング[long parameter list編]
今回は、long parameter listのリファクタリングをしていきます。
long parameter listとは
その名の通り、引数が多すぎるcode smellです。
引数が多すぎると、可読性が格段に下がります。
引数は最大でも３つまでにしておくべきと、clean codeでボブおじさんは言っています。
主に３つのソリューションがあります。
それぞれ解説していきます。
1. オブジェクトそのものの受け渡し
名前の通り、値ではなくオブジェクトそのものを引数に渡します。
package study.refactoring.longParameterList class Main { def main(args: Array[String]): Unit = { val plan = HeatingPlan(10, 20) val daysTemp = DaysRoomTemp(2) val room = Room(plan) val bottom = room.heatingPlan.bottom val top = room.heatingPlan.top if(daysTemp.withRange(bottom, top)) { println("室温が設定値を超えました。") } } } case class Room(heatingPlan: HeatingPlan) { } case class HeatingPlan(bottom: Int, top: Int) case class DaysRoomTemp(temperature: Int) { def withRange(bottom: Int, top: Int): Boolean = { this.temperature < heatingPlan.bottom && this.temperature > heatingPlan.top } }
daysTemp.withRange(bottom, top) で、引数に複数のものを渡してしまっています。
そもそもbottomとtopは、HeatingPlanオブジェクトの値なので、バラバラで渡すのではなく、HeatingPlanオブジェクト自信を引数で渡して、 関数の中で、バラバラにするのが良さそうです。
........
if(!daysTemp.withRange(room.heatingPlan)) {
println("室温が設定値を超えました。")
}
}
}
case class DaysRoomTemp(temperature: Int) {
def withRange(heatingPlan: HeatingPlan): Boolean = {
this.temperature < heatingPlan.bottom && this.temperature > heatingPlan.top
}
}
上記のようにすることで、余分な引数の数が減りました。
また、オブジェクトを引数に渡しているので、可読性が格段に上がることがわかります。
現在の室温と、設定した室温を比べているのだなと関数の中の処理を読まなくても一目瞭然になっています。
2. パラメータオブジェクトの導入
値オブジェクトを作ることで、引数をオブジェクトにまとめることができるソリューションです。
package study.refactoring.longParameterList class Main2 { def main(args: Array[String]): Unit = { val house = new House() val heatingPlan = HeatingPlan(10, 20) if(exceedSettingHeatingPlan(house, 10, 20)) { println("過去の室温で設定外になっていると場合がありました。") } } def exceedSettingHeatingPlan(station: House, bottom: Int, top: Int): Boolean = { station.getHistoryTempList.exists(temp => { temp < bottom || temp > top }) } } class House() { def getHistoryTempList: List[Int] = List(5, 6, 7, 8) }
10 20をそのまま引数に渡しているので、これはオブジェクトにできそうです。 HeatingPlanオブジェクトを作ります。
// 10 20オブジェクトに格納する if(exceedSettingHeatingPlan(house, HeatingPlan(10, 20))) { println("過去の室温で設定外になっていると場合がありました。") } case class HeatingPlan(bottom: Int, top: Int)
exceedSettingHeatingPlanメソッドの引数受け取りの値も、HeatingPlanに変更します。
def exceedSettingHeatingPlan(station: House, heatingPlan: HeatingPlan): Boolean = {
station.getHistoryTempList.exists(temp => {
temp < heatingPlan.bottom || temp > heatingPlan.top
})
}
これで、オブジェクトを引数に持たすことができました。 可読性がだいぶ上がりましたね。
しかし、これだけだと１つ目のリファクタリングと大した違いはありません。
ここからが、パラメータオブジェクトを作成することの真価です。
作成したオブジェクトに振る舞い（関数）を持たせることができることで、パラメータオブジェクトはただのデータオブジェクトではなく、
振る舞いを持った立派なオブジェクトへと進化することができます。
def exceedSettingHeatingPlan(station: House, heatingPlan: HeatingPlan): Boolean = { station.getHistoryTempList.exists(temp => { // HeatingPlanクラスに、containsメソッドを生やしています。 heatingPlan.contains(temp) }) } case class HeatingPlan(bottom: Int, top: Int) { def contains(temp: Int): Boolean = { temp < bottom || temp > top } }
凝集度が上がり、betterな設計になってきました。
凝集度の点で、もう一箇所リファクタリングしていきます。
if(house.exceedSettingHeatingPlan(HeatingPlan(10, 20))) { println("過去の室温で設定外になっていると場合がありました。") } class House { def exceedSettingHeatingPlan(heatingPlan: HeatingPlan): Boolean = { getHistoryTempList.exists(temp => { heatingPlan.contains(temp) }) } private def getHistoryTempList: List[Int] = List(5, 6, 7, 8) }
exceedSettingHeatingPlanメソッドは、Houseクラスの責務だと思うので、HouseクラスにexceedSettingHeatingPlanメソッドを生やします。
すると、Houseクラスの凝集度は上がり、さらに exceedSettingHeatingPlanメソッドの引数も減りました。
積極的にオブジェクトを作っていくことは、いいことづくしですね。
3. フラグパラメータの削除
フラグパラメータを削除します。
フラグパラメータは、関数が何をするのかを理解するためのプロセスが煩雑になってしまい、非常に読みにくいです。
特に以下の例のように、true falseだけを引数に渡していても、それが何を表しているのかを、関数の中にまで入って読まないといけなくなるので、
非常に罪深いです。
package study.refactoring.longParameterList class Main3 { def main(args: Array[String]): Unit = { val order = Order() deliverDate(order, true) } def deliverDate(order: Order, isRush: Boolean): Unit = { if(isRush) { ??? } else { ??? } } } class Main4 { def main(args: Array[String]): Unit = { val order = Order() deliverDate(order, false) } def deliverDate(order: Any, bool: Boolean): Unit = ??? } case class Order()
deliverDateメソッドを見ただけで、trueが何を表しているのか理解できません。
このようなフラグ引数がある場合は、true falseでそれぞれ新しい関数を作ってあげ、それぞれに適した関数名をつけてあげることで、
可読性が上がります。
package study.refactoring.longParameterList class Main3 { def main(args: Array[String]): Unit = { val order = Order() rushDeliverDate(order) } def rushDeliverDate(order: Order): Unit = { ??? } } class Main4 { def main(args: Array[String]): Unit = { val order = Order() BasicDeliverDate(order) } def BasicDeliverDate(order: Order): Unit = ??? } case class Order()
これでフラグ引数はなくなり、可読性が上がりました。
まとめ
積極的にオブジェクトを作成してあげることが、１つのキーになるかなと思います。
最後までありがとうございました。
次は、feature enzyについて書いていきたいと思います。
参考
もしこの世にDIコンテナがなかったら。
この記事はただの集団 Advent Calendar 2019の9日目の記事です。
ネット上に非常に多くの記事があるので、DIコンテナの話はほとんどしません。
この記事では、DIコンテナの目的である、
オブジェクトの生成と利用の分離
制御の反転
の話をしますmm
もしこの世にDIコンテナが存在していなくて、DIコンテナが行なっていることを手動でしなければいけない場合、どのような実装が考えられるのか、実際にコードを書いてみます。
前提知識
DIコンテナとは
アプリケーションにDI(Dependency Injection: 依存性注入)機能を提供するフレームワーク。
詳しくは、以下の記事にまとめられているので参照してください。
DIコンテナとは DI・DIコンテナ、ちゃんと理解出来てる・・？ -Qiita
何をしてくれるのか
そもそもDIはなぜ必要なのでしょうか？ DIの非常に大事な目的として、以下の目的があります。
1. オブジェクトの生成と利用を分離する
これは、関心ごとの分離と言うこともできます。
2. 制御の反転
SOLIDで言うと、依存性逆転の原則です。
これらの目的を実現するための１つの方法としてDIがあります。
DI以外にもこれらを実現するものとして、様々な方法があります。
この記事では、上記の目的を実現する様々な方法の中のいくつかを実際にコードベースで紹介していきます。
「オブジェクトの生成と利用を分離」しないと何がだめ？
1. 単一責任の分離ができていないので、テストが非常にしにくくなる。
2. オブジェクトの生成ロジックがそこらへんに散らばってしまい、時に著しい重複を生み出してしまうことがある。
searchメソッドをテストすると、実際にJobRepositoryImpleWithMysqlインスタンスが作られてしまうので、 もし、JobRepositoryImpleWithMysqlがDBとの連携を行なっていた場合に、非常にテストがやりずらい。
class SearchUseCase { def search = { val repositoryImpleWithMysql = new JobRepositoryImpleWithMysql ........... } }
「制御の反転」しないと何がだめ？
1. 具象クラスに依存することで、具象クラスが変更されるとごっそり修正が必要になる。
2. 具象クラスがDBとのコネクションを行なっている場合など、テストがしにくい。
JobRepositoryImpleWithMysqlをjobRepositoryImpleWithElasticSearchに変更すると、 利用側（SearchUseCase）でも、修正が必要になってしまう。
class SearchUseCase { def search = { val repositoryImpleWithMysql = new JobRepositoryImpleWithMysql ........... } } class JobRepositoryImpleWithMysql { def getJobs: Seq[String] = Seq("job1", "job2") }
「オブジェクトの生成と利用を分離」を実現するパターン
大きく分けて３つのパターンがあります。 この中で、太文字のものを実際のコードで紹介していきます。
- 依存性注入（DI）
- コンストラクタパターン
- cake pattern
- ファクトリ
- abstract factory pattern
- mainの分離
cake patternとabstract factory patternは「制御の反転」も実現しているので、まずコンストラクタパターンのみ見ていきます。
cake patternは、scala特有のパターンらしい。（参照）
コンストラクタパターン
package study.ConstractPattern class Main { def main(args: Array[String]): Unit = { val useCase = new SearchUseCase(new JobRepositoryImpleWithMysql) useCase.search } } class SearchUseCase(jobRepositoryImpleWithMysql: JobRepositoryImpleWithMysql) { // コンストラクタ引数で、SearchUseCaseにJobRepositoryImpleを委譲している。 // searchUseCaseないでnew JobRepositoryImpleして、オブジェクトを注入するのではなく、 // SearchUseCaseオブジェクトを宣言している箇所から、注入することによって // 外部から依存性注入をすることができている。 // 外部から、これ使えって！オブジェクトを注入している。 val jobRepositoryImpleInstance = jobRepositoryImpleWithMysql def search: Seq[String] = { jobRepositoryImpleInstance.getJobs } } trait JobRepository { def getJobs: Seq[String] } class JobRepositoryImpleWithMysql extends JobRepository { override def getJobs: Seq[String] = Seq("job1", "job2") } class jobRepositoryImpleWithElasticSearch extends JobRepository { override def getJobs: Seq[String] = Seq("job1", "job2") }
コメントでも書いている通り、コンストラクタ引数として利用するオブジェクトを注入することによって、オブジェクトの生成と利用を分離することができています。
できること
- オブジェクトの構成ロジックと、通常の実行処理を分離することによって、単一責任の法則を守れている。
- なので、テストがしやすくなる
できないこと
- 結局、アプリケーション実行タイミングで、newしてオブジェクトを作成しないといけないので、そこに責務が集中する。
- 具象クラスがuseCase側に見えてしまっている（直接、具象クラスを呼び出している）ので、具象クラスの実装が変わると、useCase側にも影響が及んでしまう。（これだけでは、制御の反転はできていない）
ex) jobRepositoryImpleWithMysqlが、jobRepositoryImpleWithElasticSearchに変わると、ごっそり変更を加えないといけなくなる。
制御の反転を実現するパターン
以下の方法があります。
- 依存性注入（DI）
- cake pattern
- ファクトリ
- abstract factory pattern
abstract factory pattern
package study.abstractFactoryPattern class Main { def main(args: Array[String]): Unit = { val searchUseCase = new SearchUseCase searchUseCase.search } } // 使用側は、具象クラスについて知る必要がなくなる。 // アプリケーション側でオブジェクトの生成に関する制御を行う class SearchUseCase { def search = { val repository: JobRepository = JobRepositoryFactory.createJobRepository repository.getJobs } } // abstract factory trait AbstractFactory { def createJobRepository: JobRepository } // abstract product trait JobRepository { def getJobs: Seq[String] } // concrete factory object JobRepositoryFactory extends AbstractFactory { override def createJobRepository: JobRepository = new JobRepositoryImple } // concrete product class JobRepositoryImple extends JobRepository { override def getJobs: Seq[String] = Seq("job1", "job2") }
できること
- factory.createJobRepositoryで、オブジェクト生成部分を実行処理を分離することで、関心ごとの分離を実現できている。
- 以下の例のように、永続化層を呼び出す側は、具象クラスを知る必要がなくなる。
- repository.getJobsなので、呼び出し側は具象クラスが変更されても（例えば、JobRepositoryImpleWithMysql -> JobRepositoryImpleWithElasticSearch） 影響を受けない。
cake pattern
package study.cakePattern class Main { def main(args: Array[String]): Unit = { // searchUseCase自体をインジェクトしている val useCase = ComponentRegistry.searchUseCase useCase.search } } // 抽象のコンポーネント // componentで囲って、それぞれ名前空間を作ってあげる trait JobRepositoryComponent { val jobRepository: JobRepository trait JobRepository { def search: Unit } } // 具象のコンポーネント // componentで囲って、それぞれ名前空間を作ってあげる trait JobRepositoryComponentImple extends JobRepositoryComponent { class JobRepositoryImple extends JobRepository { override def search: Unit = println("create user") } } // クライアントのコンポーネント // 自分型アノテーションを使って、UserRepositoryへの依存性を宣言する trait SearchUseCaseComponent { self: JobRepositoryComponent => class SearchUseCase { def search = { // このjobRepositoryがインジェクトされたい self.jobRepository.search } } } // インジェクター（DIコンテナの役割） // SearchUseCaseComponentは、自分型アノテーションでJobRepositoryComponentを宣言しているので // JobRepositoryComponentImpleもmixinしてあげないといけない object ComponentRegistry extends SearchUseCaseComponent with JobRepositoryComponentImple { override val jobRepository = new JobRepositoryImple val searchUseCase = new SearchUseCase }
できること
SearchUseCaseクラスのsearchメソッドの中で、具象クラス（JobRepositoryImple）に依存するのではなく、抽象クラス（JobRepository）に依存することで、制御の反転が実現できています。
まとめ
以上のようにいくつかのやり方を見ていきました。 DIコンテナを使うことで、内部的に複雑な処理を行なってくれるのでよりDIコンテナの便利さを実感しました。
DIコンテナを導入するデメリットはあるのかな？
最後までありがとうございましたmm
参考記事
沈思黙考：デザインパターン(Abstract Factory パターン) - Qiita
実戦での Scala: Cake パターンを用いた Dependency Injection (DI) | eed3si9n
code smell リファクタリング[long method編]
記念すべき１つ目は、long methodのリファクタリングを解説をしていきます。
long methodとは
名前の通り、メソッドに含まれるコードの量が多すぎるということです。
なぜcode smellなのか？
そもそもなぜメソッド内のコードの量が多すぎることが問題なのでしょうか？
2つの点が考えれると思います。
- 単純に読みにくいので処理を追いにくい
例えば、同じメソッドを100行ほどのコードが書かれていると、三行目くらいで読むのを諦めてしまいます。
- ユニットテスト（UT）がしにくいので、バグの温床になる
同じ関数内で複数の処理をしていると、UTがしにくいので、網羅されていない、ふとした瞬間にバグの温床になります。
以下の例では、createUrlメソッドの中で、redisから値を取得する処理と、実際にUrlを作成する処理の二つが含まれています。
def createUrl(key: String): String = { val keyword = redisClient.getCaches(key).orElse("hoge") s"http://localhost:9000?q=${keyword}" }
long methodになる原因
ではなぜ関数が長くなってしまうのでしょうか？
主に、以下の2つの原因が考えられます。
- 関数内で複数の処理をしている（複数の概念を持ってしまっている）
- 関数内の処理の抽象度が揃っていない
ソリューション
上記にあげた2つの原因を１つずつ潰していけば、long methodのcode smellは無くなりそうです。
long methodの解消方法はたくさんあるので、それぞれどの方法を当てはめるかは、ぶち当たってるlong methodよります。なので、一概これ！！！とは言えないですが、
一般的に、long methodの解消法の大きな概念として以下１つのことを行えば解消できると思います。
関数を抽出する
この関数を抽出すると言っても、様々な捉え方があります。
単純に同じクラス内で、privateメソッドに抽出する。
- before
メソッド内の処理が複雑化しており、また具体的な処理がifの中だけで書かれているので抽象度が揃っていないので、読みにくくなっています。
def search(searchResult: SearchResult): String = { if(searchResult.totalHits > 0 || searchResult.result.nonEmpty) { Ok( createResponse(.....) ) } else { NotFound } }
- after
ifの中身をprivateメソッドに持って行くとメソッド内の抽象度が揃って、読みやすくなりました。
def search(searchResult: SearchResult): String = { if(existSearchResult(searchResult)) { Ok( createResponse(.....) ) } else { NotFound } } private def existSearchResult(searchResult: SearchResult): Boolean = { searchResult.totalHits > 0 || searchResult.result.nonEmpty }
他のクラスにメソッドを持って行く。
先ほどの例を凝集度の観点でさらにリファクタリングすると。
def search(searchResult: SearchResult): String = { if(searchResult.hasContent) { Ok( createResponse(.....) ) } else { NotFound } } case class SearchResult( totalHits: Long, result: Option[Result] ) { def hasContent: Boolean = totalHits > 0 || result.isDefined }
SearchResultの凝集度が上がって、さらに読みやすくなりました。
凝集度に関しては、次のlarge classリファクタリングの記事で解説します。
複数の処理をしている場合に、それぞれの処理をメソッドに切り分けてあげる。
- before
最初の例では、一つのメソッドの中で複数の処理を行ってしまっています。
def createUrl(key: String): String = { val keyword = redisClient.getCaches(key).orElse("hoge") s"http://localhost:9000?q=${keyword}" }
- after
URL作成処理と、redisからkeywordを取得する処理を完全に分けます。
createUrlメソッドの中で、getKeyWordを呼び出すのではなく、
それぞれのメソッドの呼び出し元（今回の場合はmainメソッド）で、それぞれのメソッドを呼び出します。
そうすることで、処理の抽象度が揃い格段に読みやすくなります。
def main() { val keyword = getKeyWord(key) createUrl(keyword) } private getKeyWord(key: String): String = { redisClient.getCaches(key).orElse("hoge") } private def createUrl(key: String): String = { s"http://localhost:9000?q=${keyword}" }
