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デ開発をしようと改めて思いました。
最後まで読んでいただきありがとうございます。
