リファクタリングとは
wikipedia：リファクタリングの言葉を借りると、「リファクタリング (refactoring) とはコンピュータプログラミングにおいて、プログラムの外部から見た動作を変えずにソースコードの内部構造を整理すること。」です。
簡単に言うと、汚いコードは修正するだけでも非常にコストが掛かってしまうから、リファクタリングの様々な手法を用いて綺麗なコードを保っておくことで、改修が容易にできかつバグの入り込み辛い状況をキープしようという試みです。
また、リファクタリングを適切に行うことで、プログラムの部品化が促進されることになるため、プログラムを再利用できる可能性が高くなります。
本稿の目的
現在私は、「リファクタリング 既存のコードを安全に改善する」という書籍を読んでいるのですが、その書籍の一番最初に、イケてないコードを例にリファクタリングを行い、**リファクタリングとはなにか？どんなメリットがあるのか？**という興味を持たせる章が50ページほど割いて記載されています。
ここで例に載っているソースを電子データでないかを探してみたのですが、見つけられませんでした。（ここに昔の書籍の情報はありました）
そこで、本稿では例題として、書籍で紹介されている「イケてないサンプルコード」を掲載した上で、まずはそれを私が独力でリファクタリングした結果を紹介し、書籍の考え方と違っていた部分・合っていた部分を紹介したいと思います。
まぁ、「私のようなリファクタリング初心者（いつも感覚的に実施してしまっているという意味）」が「イケてないコードを自分なりにリファクタリングを実施」してみて、自己採点して、自分自身に不足している考え方などを整理してみたという感じです。
もしリファクタリングについて学習しようとしている方は、同じように「イケてないサンプルコード」を独力でリファクタリングしてみて、自身に不足している知識やテクニックを重点的に見に付けると、効率的な学習ができるのではないかな？と思います。（私の独力リファクタリングは役に立たないかもしれませんが、基ネタとテストコードは役に立つと思いますよ）
本稿は、以下のような構成にしています
1.初めにリファクタリング前の基ソースを提示
2.step#1～step#8にかけて独力でリファクタリングしていった証跡を提示
3.最後に書籍で実際に紹介していたリファクタリングと照合してOKな部分・NGな部分・不足している部分をまとめる
※書籍の紹介したいわけではありませんので、書籍がリファクタリングした結果のコードは掲載していません。書籍が紹介するリファクタリング結果のコードを知りたい場合は書籍を参照していただくようお願いいたします。
イケてないコードのサンプル
紹介されていたイケてないコードは、以下の様な簡単なプログラムでした。
機能：「レンタルするMovieの料金を計算して計算書を印刷する」
具体的には、「どのMovieを借りるか？」「何日借りるか？」という情報を入力すると、「料金が計算され」、「一般向け・子供向け・新作が判定され」「レンタルポイントが計算され」、「最後にそれらを印刷する」というものです。
イケてないコードの基ネタ
実際に紹介されていたコード（privateな変数名に"_"がついていたのですが、getter、setterの自動生成時に"_"がつくのがいやだったので、取っ払っています）
まずは、レンタル対象のMovieを表現するクラスです。
追記：これは、eclipseの設定で、フィールド名のprefixを指定することで、自動生成時に"_"をつけないようにすることが可能です。。
Movie.java
public class Movie {
public static final int CHILDRENS = 2;
public static final int REGULAR = 0;
public static final int NEW_RELEASE = 1;
private String title;
private int priceCode;
public Movie(String title, int priceCode) {
this.title = title;
this.priceCode = priceCode;
}
public int getPriceCode() {
return priceCode;
}
public void setPriceCode(int priceCode) {
this.priceCode = priceCode;
}
public String getTitle() {
return title;
}
}
次に、レンタル日数を表すクラスです。どのMovieを何日借りるか？という情報を管理します。
Rental.java
public class Rental {
private Movie movie;
private int daysRented;
public Rental(Movie movie, int daysRented) {
this.movie = movie;
this.daysRented = daysRented;
}
public Movie getMovie() {
return movie;
}
public int getDaysRented() {
return daysRented;
}
}
最後に、お客様を表現するクラスです。修正してくださいと、言わんばかりのクラスになっていますね（笑）
Customer.java
import java.util.Enumeration;
import java.util.Vector;
public class Customer {
private String name;
private Vector<Rental> rentals = new Vector<>();
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentals.addElement(arg);
}
public String getName() {
return name;
}
public String statement() {
double totalAmount = 0;
int frequentREnterPoints = 0;
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while( rentals.hasMoreElements() ) {
double thisAmount = 0;
Rental each = (Rental) rentals.nextElement();
// 1行ごとに金額を計算
switch( each.getMovie().getPriceCode() ) {
case Movie.REGULAR:
thisAmount += 2;
if( each.getDaysRented() > 2)
thisAmount += ( each.getDaysRented() -2) * 1.5;
break;
case Movie.NEW_RELEASE:
thisAmount += each.getDaysRented() * 3;
break;
case Movie.CHILDRENS:
thisAmount += 1.5;
if ( each.getDaysRented() > 3 )
thisAmount += (each.getDaysRented() -3 ) * 1.5;
break;
}
// レンタルポイントを加算
frequentREnterPoints ++;
// 新作を二日以上借りた場合はボーナスポイント
if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) &&
each.getDaysRented() > 1) frequentREnterPoints ++;
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
テストコード
リファクタリングをやって見る前に、まずは仕様を壊していないことを担保するためのテストコードを作りました。（書籍にはテストコードを最初に作ること！！と記載されていたのですが、具体的なソースは用意してくれていませんでした。。。詳細の説明は割愛しますが、全ての種類のMovieを１～２種類レンタルした場合を検証するテストコードにしています。
CustomerTest.java
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.*;
import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.Theory;
import org.junit.runner.RunWith;
@RunWith(Theories.class)
public class CustomerTest {
private static final String CUSTOMER_NAME = "テスト";
private Customer sut = new Customer(CUSTOMER_NAME);
@DataPoints
public static Fixture[] testDatas = {
new Fixture("movie1", Movie.NEW_RELEASE, 1),
new Fixture("movie1", Movie.NEW_RELEASE, 2),
new Fixture("movie1", Movie.NEW_RELEASE, 3),
new Fixture("movie1", Movie.NEW_RELEASE, 4),
new Fixture("movie2", Movie.CHILDRENS, 1),
new Fixture("movie2", Movie.CHILDRENS, 2),
new Fixture("movie2", Movie.CHILDRENS, 3),
new Fixture("movie2", Movie.CHILDRENS, 4),
new Fixture("movie3", Movie.REGULAR, 1),
new Fixture("movie3", Movie.REGULAR, 2),
new Fixture("movie3", Movie.REGULAR, 3),
new Fixture("movie3", Movie.REGULAR, 4),
null
};
@Theory
public void test(Fixture fixture1, Fixture fixture2) {
// 【テスト条件のセットアップ】
if( fixture1 != null ) {
sut.addRental( fixture1.rental );
}
if( fixture2 != null ) {
sut.addRental( fixture2.rental );
}
// 【テスト実施】
String result = sut.statement();
// 【テスト結果の検証】
// 期待値作成
String expected = "Rental Record for " + CUSTOMER_NAME + "\n";
expected += buildAmountPartString(fixture1);
expected += buildAmountPartString(fixture2);
double totalAmount = calcTotalAmount(fixture1,fixture2);
expected += "Amount owed is " + String.valueOf(totalAmount) + "\n";
int frequentREnterPoints = calcPoint(fixture1,fixture2);
expected += "You earned " + String.valueOf(frequentREnterPoints) + " frequent renter points";
assertThat( result, is(expected) );
}
private double calcTotalAmount(Fixture... fixtures) {
double totalAmount = 0;
for(Fixture fixture : fixtures) {
if( fixture == null ) continue;
totalAmount += fixture.amount;
}
return totalAmount;
}
/**
* ボーナスポイントを計算する。
* @param fixtures
* @return
*/
private int calcPoint(Fixture... fixtures) {
int frequentREnterPoints = 0;
for(Fixture fixture : fixtures) {
if( fixture == null ) continue;
frequentREnterPoints++;
// 新作を二日以上借りた場合はボーナスポイント
if( (fixture.movie.getPriceCode() == Movie.NEW_RELEASE ) &&
fixture.rental.getDaysRented() > 1) {
frequentREnterPoints++;
}
}
return frequentREnterPoints;
}
/**
* Fixtureを元に、個別のmovieの料金を示す文字列を作成する。
* @param fixture
* @return
*/
private String buildAmountPartString(Fixture fixture) {
if( fixture == null ) {
return "";
}
final String amountPartFormat = "\t%s\t%s\n";
String movieTitle = fixture.movie.getTitle();
String amount = String.valueOf(fixture.amount);
return String.format(amountPartFormat, movieTitle, amount);
}
/**
* テスト用データをひとまとめに扱うためのクラス
*/
static class Fixture {
private Movie movie;
private Rental rental;
private double amount;
public Fixture(String movieTitle, int priceCode, int daysRented) {
this.movie = new Movie(movieTitle, priceCode);
this.rental = new Rental(movie, daysRented);
// 一つのmovieの料金を計算
amount = 0;
switch(priceCode) {
case Movie.REGULAR:
amount += 2;
if( daysRented > 2)
amount += (daysRented - 2 ) * 1.5;
break;
case Movie.NEW_RELEASE:
amount += daysRented * 3;
break;
case Movie.CHILDRENS:
amount += 1.5;
if ( daysRented > 3 )
amount += (daysRented -3 ) * 1.5;
break
;
}
}
}
}
独力でリファクタリングしてみます
それでは、特に書籍を読まない状態で独力でリファクタリングしてみた軌跡を紹介します。
参考になるかわかりませんが、なるべく自分自身の思考プロセスがわかるように細かく記載します。（後ほど、書籍の情報と照合して、よかったところ悪かったところ、そもそも知らない知識をまとめます。）
あとで書籍と照合してみて、リファクタリング力ゼロだったらどうしよう。。。なんて不安がいっぱいです。。。
step1
まずは、ぱっと見で気持ち悪いCutomerのstatementをどうしてやろうかなぁと思いました。
いろいろ気持ち悪いところはあるのですが、最も臭いと感じた「一つのレンタルの料金を計算するロジックを外出ししました。
Customer.java#1
import java.util.Enumeration;
import java.util.Vector;
public class Customer {
private String name;
private Vector<Rental> rentals = new Vector<>();
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentals.addElement(arg);
}
public String getName() {
return name;
}
/**
* 一つのレンタルに対する料金を計算する★メソッドを外出ししました★
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private double calcTargetRentalAmount(int targetPriceCode, int targetDaysRented) {
double amount = 0;
switch (targetPriceCode) {
case Movie.REGULAR:
amount += 2;
if(targetDaysRented > 2) amount += ( targetDaysRented - 2 ) * 1.5;
break;
case Movie.NEW_RELEASE:
amount += targetDaysRented * 3;
break;
case Movie.CHILDRENS:
amount += 1.5;
if(targetDaysRented > 3) amount += ( targetDaysRented - 3 ) * 1.5;
break;
}
return amount;
}
public String statement() {
double totalAmount = 0;
int frequentREnterPoints = 0;
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while( rentals.hasMoreElements() ) {
Rental each = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する★メソッドを外出ししました★
int targetPriceCode = each.getMovie().getPriceCode();
int targetDaysRented = each.getDaysRented();
double thisAmount = calcTargetRentalAmount(targetPriceCode, targetDaysRented);
// レンタルポイントを加算
frequentREnterPoints ++;
// 新作を二日以上借りた場合はボーナスポイント
if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) &&
each.getDaysRented() > 1) frequentREnterPoints ++;
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
step2
次に、ポイントの計算部分を別メソッドに切り出しました。
Customer.java#2
import java.util.Enumeration;
import java.util.Vector;
public class Customer {
private String name;
private Vector<Rental> rentals = new Vector<>();
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentals.addElement(arg);
}
public String getName() {
return name;
}
/**
* 一つのレンタルに対する料金を計算する
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private double calcTargetRentalAmount(int targetPriceCode, int targetDaysRented) {
double amount = 0;
switch (targetPriceCode) {
case Movie.REGULAR:
amount += 2;
if(targetDaysRented > 2) amount += ( targetDaysRented - 2 ) * 1.5;
break;
case Movie.NEW_RELEASE:
amount += targetDaysRented * 3;
break;
case Movie.CHILDRENS:
amount += 1.5;
if(targetDaysRented > 3) amount += ( targetDaysRented - 3 ) * 1.5;
break;
}
return amount;
}
/**
* 一つのRentalに対するポイントを計算する★メソッドを外出ししました★
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private double clacTargetRentalPoint(int targetPriceCode, int targetDaysRented) {
// 通常1ポイント
int targetRentalPoint = 1;
// 追加のボーナスポイントが必要な場合★メソッドを外出ししました★
if( needAdditionalPoint(targetPriceCode,targetDaysRented) ) {
targetRentalPoint++;
}
return targetRentalPoint;
}
/**
* 追加でポイントを付与する必要があるかどうかを判定する。★メソッドを外出ししました★
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private boolean needAdditionalPoint(int targetPriceCode, int targetDaysRented) {
if( targetPriceCode == Movie.NEW_RELEASE ) {
if( targetDaysRented > 1 ) {
return true;
}
}
return false;
}
public String statement() {
double totalAmount = 0;
int frequentREnterPoints = 0;
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while( rentals.hasMoreElements() ) {
Rental each = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する★メソッドを外出ししました★
int targetPriceCode = each.getMovie().getPriceCode();
int targetDaysRented = each.getDaysRented();
double thisAmount = calcTargetRentalAmount(targetPriceCode, targetDaysRented);
// レンタルポイントを加算★メソッドを外出ししました★
frequentREnterPoints += clacTargetRentalPoint(targetPriceCode,targetDaysRented);
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
step3
priceCodeに応じた金額値は、Customer内部で計算していましたが、そもそもその価格はCustomerではなく、Movieに依存するものであるため、価格は、Movieに持たせるように変更しました。
Movie.java#3
public class Movie {
private String title;
private MovieType priceCode;
public Movie(String title, MovieType priceCode) {
this.title = title;
this.priceCode = priceCode;
}
public MovieType getPriceCode() {
return priceCode;
}
public void setPriceCode(MovieType priceCode) {
this.priceCode = priceCode;
}
public String getTitle() {
return title;
}
}
// ★priceCodeをstatic intで持っていましたが、
// codeに対する単価とセットで扱うように修正★
enum MovieType {
NEW_RELEASE(3),
REGULAR(2),
CHILDRENS(1.5);
private MovieType(double amount) {
this.amount = amount;
}
public final double amount;
}
Customer.java#3
import java.util.Enumeration;
import java.util.Vector;
public class Customer {
private String name;
private Vector<Rental> rentals = new Vector<>();
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentals.addElement(arg);
}
public String getName() {
return name;
}
/**
* 一つのレンタルに対する料金を計算する
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private double calcTargetRentalAmount(MovieType targetPriceCode, int targetDaysRented) {
double amount = targetPriceCode.amount;
switch (targetPriceCode) {
case REGULAR:
if(targetDaysRented > 2) amount += ( targetDaysRented - 2 ) * 1.5;
break;
case NEW_RELEASE:
amount += (targetDaysRented - 1) * 3;
break;
case CHILDRENS:
if(targetDaysRented > 3) amount += ( targetDaysRented - 3 ) * 1.5;
break;
}
return amount;
}
/**
* 一つのRentalに対するポイントを計算する
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private double clacTargetRentalPoint(MovieType targetPriceCode, int targetDaysRented) {
// 通常1ポイント
int targetRentalPoint = 1;
// 追加のボーナスポイントが必要な場合
if( needAdditionalPoint(targetPriceCode,targetDaysRented) ) {
targetRentalPoint++;
}
return targetRentalPoint;
}
/**
* 追加でポイントを付与する必要があるかどうかを判定する。
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private boolean needAdditionalPoint(MovieType targetPriceCode, int targetDaysRented) {
if( targetPriceCode == MovieType.NEW_RELEASE ) {
if( targetDaysRented > 1 ) {
return true;
}
}
return false;
}
public String statement() {
double totalAmount = 0;
int frequentREnterPoints = 0;
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while( rentals.hasMoreElements() ) {
Rental each = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する
MovieType targetPriceCode = each.getMovie().getPriceCode();
int targetDaysRented = each.getDaysRented();
double thisAmount = calcTargetRentalAmount(targetPriceCode, targetDaysRented);
// レンタルポイントを加算
frequentREnterPoints += clacTargetRentalPoint(targetPriceCode,targetDaysRented);
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
今回の修正はテストコードにもあたりがあるので、修正
CustomerTest.java#3
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.*;
import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.Theory;
import org.junit.runner.RunWith;
@RunWith(Theories.class)
public class CustomerTest {
private static final String CUSTOMER_NAME = "テスト";
private Customer sut = new Customer(CUSTOMER_NAME);
@DataPoints
public static Fixture[] testDatas = {
new Fixture("movie1", MovieType.NEW_RELEASE, 1),
new Fixture("movie1", MovieType.NEW_RELEASE, 2),
new Fixture("movie1", MovieType.NEW_RELEASE, 3),
new Fixture("movie1", MovieType.NEW_RELEASE, 4),
new Fixture("movie2", MovieType.CHILDRENS, 1),
new Fixture("movie2", MovieType.CHILDRENS, 2),
new Fixture("movie2", MovieType.CHILDRENS, 3),
new Fixture("movie2", MovieType.CHILDRENS, 4),
new Fixture("movie3", MovieType.REGULAR, 1),
new Fixture("movie3", MovieType.REGULAR, 2),
new Fixture("movie3", MovieType.REGULAR, 3),
new Fixture("movie3", MovieType.REGULAR, 4),
null
};
@Theory
public void test(Fixture fixture1, Fixture fixture2) {
// 【テスト条件のセットアップ】
if( fixture1 != null ) {
sut.addRental( fixture1.rental );
}
if( fixture2 != null ) {
sut.addRental( fixture2.rental );
}
// 【テスト実施】
String result = sut.statement();
// 【テスト結果の検証】
// 期待値作成
String expected = "Rental Record for " + CUSTOMER_NAME + "\n";
expected += buildAmountPartString(fixture1);
expected += buildAmountPartString(fixture2);
double totalAmount = calcTotalAmount(fixture1,fixture2);
expected += "Amount owed is " + String.valueOf(totalAmount) + "\n";
int frequentREnterPoints = calcPoint(fixture1,fixture2);
expected += "You earned " + String.valueOf(frequentREnterPoints) + " frequent renter points";
assertThat( result, is(expected) );
}
private double calcTotalAmount(Fixture... fixtures) {
double totalAmount = 0;
for(Fixture fixture : fixtures) {
if( fixture == null ) continue;
totalAmount += fixture.amount;
}
return totalAmount;
}
/**
* ボーナスポイントを計算する。
* @param fixtures
* @return
*/
private int calcPoint(Fixture... fixtures) {
int frequentREnterPoints = 0;
for(Fixture fixture : fixtures) {
if( fixture == null ) continue;
frequentREnterPoints++;
// 新作を二日以上借りた場合はボーナスポイント
if( (fixture.movie.getPriceCode() == MovieType.NEW_RELEASE ) &&
fixture.rental.getDaysRented() > 1) {
frequentREnterPoints++;
}
}
return frequentREnterPoints;
}
/**
* Fixtureを元に、個別のmovieの料金を示す文字列を作成する。
* @param fixture
* @return
*/
private String buildAmountPartString(Fixture fixture) {
if( fixture == null ) {
return "";
}
final String amountPartFormat = "\t%s\t%s\n";
String movieTitle = fixture.movie.getTitle();
String amount = String.valueOf(fixture.amount);
return String.format(amountPartFormat, movieTitle, amount);
}
/**
* テスト用データをひとまとめに扱うためのクラス
*/
static class Fixture {
private Movie movie;
private Rental rental;
private double amount;
public Fixture(String movieTitle, MovieType priceCode, int daysRented) {
this.movie = new Movie(movieTitle, priceCode);
this.rental = new Rental(movie, daysRented);
// 一つのmovieの料金を計算
amount = 0;
switch(priceCode) {
case REGULAR:
amount += priceCode.amount;
if( daysRented > 2)
amount += (daysRented - 2 ) * 1.5;
break;
case NEW_RELEASE:
amount += daysRented * 3;
break;
case CHILDRENS:
amount += 1.5;
if ( daysRented > 3 )
amount += (daysRented -3 ) * 1.5;
break
;
}
}
}
}
step4
延長料金の計算方法が、MovieTypeに応じて個別に実施していましたが、MovieTypeによってレンタルできるデフォルトの日数が決まっており、それを超えた場合は、MovieTypeによって決まる追加料金を加算するようにしてみました。こうすることで、不要なswitch～caseを削除することができるので、行数が少なくてすみます。
Movie.java#4
public class Movie {
private String title;
private MovieType priceCode;
public Movie(String title, MovieType priceCode) {
this.title = title;
this.priceCode = priceCode;
}
public MovieType getPriceCode() {
return priceCode;
}
public void setPriceCode(MovieType priceCode) {
this.priceCode = priceCode;
}
public String getTitle() {
return title;
}
}
enum MovieType {
NEW_RELEASE(3, 1, 3),
REGULAR(2, 2, 1.5),
CHILDRENS(1.5, 3, 1.5);
private MovieType(double amount, int defaultRentalTerm, double additionalAmount) {
this.amount = amount;
this.defaultRentalTerm = defaultRentalTerm;
this.additionalAmount = additionalAmount;
}
// MovieTypeに対応した単価
public final double amount;
// 延長料金が発生しない日数★延長料金計算用の情報をMovieTypeに持たせる★
public final int defaultRentalTerm;
// 延長が発生した場合の単価★延長料金計算用の情報をMovieTypeに持たせる★
public final double additionalAmount;
}
Customer.java#4
import java.util.Enumeration;
import java.util.Vector;
public class Customer {
private String name;
private Vector<Rental> rentals = new Vector<>();
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentals.addElement(arg);
}
public String getName() {
return name;
}
/**
* 一つのレンタルに対する料金を計算する
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private double calcTargetRentalAmount(MovieType targetPriceCode, int targetDaysRented) {
double amount = targetPriceCode.amount;
// ★延長料金の計算方法を統一的になるように変更★
int defaultRentalTerm = targetPriceCode.defaultRentalTerm;
double additionalAmount = targetPriceCode.additionalAmount;
if(targetDaysRented > defaultRentalTerm) {
int additionalTerm = targetDaysRented - defaultRentalTerm;
amount += additionalTerm * additionalAmount;
}
return amount;
}
/**
* 一つのRentalに対するポイントを計算する
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private double clacTargetRentalPoint(MovieType targetPriceCode, int targetDaysRented) {
// 通常1ポイント
int targetRentalPoint = 1;
// 追加のボーナスポイントが必要な場合
if( needAdditionalPoint(targetPriceCode,targetDaysRented) ) {
targetRentalPoint++;
}
return targetRentalPoint;
}
/**
* 追加でポイントを付与する必要があるかどうかを判定する。
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private boolean needAdditionalPoint(MovieType targetPriceCode, int targetDaysRented) {
if( targetPriceCode == MovieType.NEW_RELEASE ) {
if( targetDaysRented > 1 ) {
return true;
}
}
return false;
}
public String statement() {
double totalAmount = 0;
int frequentREnterPoints = 0;
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while( rentals.hasMoreElements() ) {
Rental each = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する
MovieType targetPriceCode = each.getMovie().getPriceCode();
int targetDaysRented = each.getDaysRented();
double thisAmount = calcTargetRentalAmount(targetPriceCode, targetDaysRented);
// レンタルポイントを加算
frequentREnterPoints += clacTargetRentalPoint(targetPriceCode,targetDaysRented);
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
step5
そもそも、Customer（顧客）が、料金計算をしているのが、気持ち悪いので、とりえあずRentalクラスに移動しました。（料金計算クラスを作るのもありかも）
Rental.java#5
public class Rental {
private Movie movie;
private int daysRented;
public Rental(Movie movie, int daysRented) {
this.movie = movie;
this.daysRented = daysRented;
}
public Movie getMovie() {
return movie;
}
public int getDaysRented() {
return daysRented;
}
/**
* 一つのレンタルに対する料金を計算する
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
public double calcTargetRentalAmount() {
MovieType targetPriceCode = movie.getPriceCode();
double amount = movie.getPriceCode().amount;
// 延長料金の計算
int defaultRentalTerm = targetPriceCode.defaultRentalTerm;
double additionalAmount = targetPriceCode.additionalAmount;
if(daysRented > defaultRentalTerm) {
int additionalTerm = daysRented - defaultRentalTerm;
amount += additionalTerm * additionalAmount;
}
return amount;
}
/**
* 一つのRentalに対するポイントを計算する
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
public double clacTargetRentalPoint() {
// 通常1ポイント
int targetRentalPoint = 1;
// 追加のボーナスポイントが必要な場合
if( needAdditionalPoint() ) {
targetRentalPoint++;
}
return targetRentalPoint;
}
/**
* 追加でポイントを付与する必要があるかどうかを判定する。
* @param targetPriceCode
* @param targetDaysRented
* @return
*/
private boolean needAdditionalPoint() {
MovieType targetPriceCode = movie.getPriceCode();
if( targetPriceCode == MovieType.NEW_RELEASE ) {
if( daysRented > 1 ) {
return true;
}
}
return false;
}
}
Customer.java#5
import java.util.Enumeration;
import java.util.Vector;
public class Customer {
private String name;
private Vector<Rental> rentals = new Vector<>();
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentals.addElement(arg);
}
public String getName() {
return name;
}
public String statement() {
double totalAmount = 0;
int frequentREnterPoints = 0;
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + getName() + "\n";
while( rentals.hasMoreElements() ) {
Rental each = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する★Rentalクラスに処理を委譲★
double thisAmount = each.calcTargetRentalAmount();
// レンタルポイントを加算★Rentalクラスに処理を委譲★
frequentREnterPoints += each.clacTargetRentalPoint();
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
step6
さらに、Customer（顧客）が、帳票出力を自分でやっているのも気持ち悪いので、Rentalの管理を行うRentalManagerというクラスを用意して、そいつが帳票も作ってくれるように変更。
Customer#6
public class Customer {
private String name;
private RenatalManager rentalManager = new RenatalManager();// ★レンタルの管理をRentalManagerにさせるように変更★
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentalManager.addRental(arg);// ★レンタルの管理をRentalManagerにさせるように変更★
}
public String getName() {
return name;
}
public String statement() {
return rentalManager.statement(name);// ★帳票出力をRentalManagerにさせるように変更★
}
}
RentalManager.java
import java.util.Enumeration;
import java.util.Vector;
public class RenatalManager {
private Vector<Rental> rentals = new Vector<>();
public void addRental(Rental arg) {
rentals.addElement(arg);
}
public String statement(String name) {
double totalAmount = 0;
int frequentREnterPoints = 0;
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + name + "\n";
while( rentals.hasMoreElements() ) {
Rental each = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する
double thisAmount = each.calcTargetRentalAmount();
// レンタルポイントを加算
frequentREnterPoints += each.clacTargetRentalPoint();
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
totalAmount += thisAmount;
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
##step7
さらに、帳票出力際に、料金計算やポイント計算もしていましたがレンタル情報が追加されたら確定する情報ですので、レンタル情報を追加したらポイントと合計金額を計算するように修正しました。
RentalManager.java#7
import java.util.Enumeration;
import java.util.Vector;
public class RenatalManager {
private Vector<Rental> rentals = new Vector<>();
private double totalAmount = 0;// ★レンタル情報が追加されたタイミングで反映するようにしました★
private int frequentREnterPoints = 0;// ★レンタル情報が追加されたタイミングで反映するようにしました★
public void addRental(Rental addTarget) {
rentals.addElement(addTarget);
totalAmount += addTarget.calcTargetRentalAmount();// ★レンタル情報が追加されたタイミングで反映するようにしました★
frequentREnterPoints += addTarget.clacTargetRentalPoint();// ★レンタル情報が追加されたタイミングで反映するようにしました★
}
public String statement(String name) {
Enumeration<Rental> rentals = this.rentals.elements();
String result = "Rental Record for " + name + "\n";
while( rentals.hasMoreElements() ) {
Rental each = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する
double thisAmount = each.calcTargetRentalAmount();
// この貸し出しに関する数値の表示
result += "\t" + each.getMovie().getTitle() + "\t" +
String.valueOf(thisAmount) + "\n";
}
result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
result += "You earned " + String.valueOf(frequentREnterPoints) +
" frequent renter points";
return result;
}
}
step8
独力リファクタリングとしては、最後に、出力する帳票の文字列をテンプレート化けしてみました。ライブラリ使うならVelocityとかFreeMakerとか利用して、テンプレート側に繰り返しもさせてしまうのですが、とりあえずは文字列をstatic final Stringで持たせています。
RenatalManager.java#8
import java.util.Enumeration;
import java.util.Vector;
public class RenatalManager {
private Vector<Rental> rentals = new Vector<>();
private double totalAmount = 0;
private int frequentREnterPoints = 0;
private static final String RECEIPT_HEADER_TEMPLATE = "Rental Record for %s\n";// ★レシート出力用の文字列をテンプレート化★
private static final String RECEIPT_DETAIL_TEMPLATE = "\t%s\t%s\n";// ★レシート出力用の文字列をテンプレート化★
private static final String RECEIPT_TOTALAMOUNT_TEMPLATE = "Amount owed is %s\n";// ★レシート出力用の文字列をテンプレート化★
private static final String RECEIPT_TOTALPOINT_TEMPLATE = "You earned %s frequent renter points";// ★レシート出力用の文字列をテンプレート化★
public void addRental(Rental addTarget) {
rentals.addElement(addTarget);
totalAmount += addTarget.calcTargetRentalAmount();
frequentREnterPoints += addTarget.clacTargetRentalPoint();
}
public String statement(String name) {
Enumeration<Rental> rentals = this.rentals.elements();
String result = String.format(RECEIPT_HEADER_TEMPLATE, name);// ★レシート出力用の文字列をテンプレート化★
while( rentals.hasMoreElements() ) {
Rental targetRental = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する
double thisAmount = targetRental.calcTargetRentalAmount();
// この貸し出しに関する数値の表示
result += String.format(RECEIPT_DETAIL_TEMPLATE, targetRental.getMovie().getTitle(), String.valueOf(thisAmount) );// ★レシート出力用の文字列をテンプレート化★
}
result += String.format( RECEIPT_TOTALAMOUNT_TEMPLATE, String.valueOf(totalAmount) );// ★レシート出力用の文字列をテンプレート化★
result += String.format( RECEIPT_TOTALPOINT_TEMPLATE, String.valueOf(frequentREnterPoints) );// ★レシート出力用の文字列をテンプレート化★
return result;
}
}
書籍と照合して答え合わせ
書籍側ではまず、stetement()に記述されている料金計算メソッドをamaountFor(というメソッドに分割していました。私もstep1でcalcTargetRentalAmount()というメソッドに切り出しているので同じ事をしていますね。書籍も同じく最初にやっているので、やはり最も臭いところという感覚も正しかったのでしょう（笑）
次に、書籍ではamoutFor()をRentalクラスに移動していました。さらに、amountFor()というメソッドを、getCharge()というメソッドに変更していました。
ここについても、step5でメソッドを移動しているので正解といえるでしょう。おそらくわかりやすくするためという理由でgetCharge()という名称にしているのだと思うのですが、私はcalcTargetRentalAmountというメソッド名のままにしています。
ここについては、私は意図的にgetXXという名前にしていません。なぜならgetXX()というメソッドは、単純なプロパティ参照という印象が強く、内部ロジックで計算しているというのが気持ち悪いからです。この感覚については、「リーダブルコード（O'Reilly）」に記載がありますので、興味がある方は読まれることをお勧めします。200ページ程度の良書です。
また、書籍の筆者は、なるべく一時変数をできる限り取り除くようにして、メソッドを都度呼びだすようにしていました。ここについても、私はリーダブルコードの考え方に寄っているので、都度メソッドを呼び出す方法ではなく、適切な名前の一時変数にすることが多いです。
具体的には、書籍では、thisAmount = each.getCharge()とせず、each.getChargeを都度呼びだしていたのですが、私は反対ですね。
【「この料金」はXXという】風に読み取れていたものが、【eachの料金を取得する】という風に脳が解釈するようになるので、脳内コンパイルに雑音が入る気がして嫌だからです。
さらに、書籍ではレンタルポイント計算部分を抽出するようにしていました。
私もstep2およびstep5でやっているので同じですね。
次に、書籍では一時変数を減らすということをやっていました。（ここは、私は受け入れたくなかった箇所です。）
何をしているかというと、
statement()メソッドで利用していた、totalAmountとfrequentREnterPointsの計算を、
Customer.getTotalCharge()とgetTotalFrequentRenterPoints()というメソッドに抜き出して、statement()以外からも利用できるようにしていました。
これもありだと思うのですが、呼び出すたびにループが入ることになるので、正直気持ち悪いです。。。まぁ、書籍側にも「ここで行われたリファクタリングには再考の余地があります。」とありました。
私がやったようにRentalManagerを用意して、Rentalが追加されるたびに合計金額と合計ポイントを都度計算しておいて保持しておくという方がよい気がしますね（自画自賛？）
ちなみに、このループを増やしてでも一時変数を削除するメリットとして、仮に帳票出力がhtml形式のものを追加したとしても容易に追加できると書いてありました。
私がリファクタリングしたソースではどうなるかやってみました。
Customer.java#9
public class Customer {
private String name;
private RenatalManager rentalManager = new RenatalManager();
public Customer (String name) {
this.name = name;
}
public void addRental(Rental arg) {
rentalManager.addRental(arg);
}
public String getName() {
return name;
}
public String statement() {
return rentalManager.statement(name);
}
// ★メソッド追加★
public String htmlStatement() {
return rentalManager.htmlStatement(name);
}
}
RenatalManager.java#9
import java.util.Enumeration;
import java.util.Vector;
public class RenatalManager {
private Vector<Rental> rentals = new Vector<>();
private double totalAmount = 0;
private int frequentREnterPoints = 0;
private static final String RECEIPT_HEADER_TEMPLATE = "Rental Record for %s\n";
private static final String RECEIPT_DETAIL_TEMPLATE = "\t%s\t%s\n";
private static final String RECEIPT_TOTALAMOUNT_TEMPLATE = "Amount owed is %s\n";
private static final String RECEIPT_TOTALPOINT_TEMPLATE = "You earned %s frequent renter points";
public void addRental(Rental addTarget) {
rentals.addElement(addTarget);
totalAmount += addTarget.calcTargetRentalAmount();
frequentREnterPoints += addTarget.clacTargetRentalPoint();
}
public String statement(String name) {
Enumeration<Rental> rentals = this.rentals.elements();
String result = String.format(RECEIPT_HEADER_TEMPLATE, name);
while( rentals.hasMoreElements() ) {
Rental targetRental = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する
double thisAmount = targetRental.calcTargetRentalAmount();
// この貸し出しに関する数値の表示
result += String.format(RECEIPT_DETAIL_TEMPLATE, targetRental.getMovie().getTitle(), String.valueOf(thisAmount) );
}
result += String.format( RECEIPT_TOTALAMOUNT_TEMPLATE, String.valueOf(totalAmount) );
result += String.format( RECEIPT_TOTALPOINT_TEMPLATE, String.valueOf(frequentREnterPoints) );
return result;
}
// 以下のようにhtml出力用の定数とメソッドを追加
private static final String RECEIPT_HEADER_HTML_TEMPLATE = "<H1>Rentals for <EM>%s</EM></H1><P>\n";
private static final String RECEIPT_DETAIL_HTML_TEMPLATE = "%s: %s<BR>\n";
private static final String RECEIPT_TOTALAMOUNT_HTML_TEMPLATE = "<P>You owe <EM>%s</EM><P>\n";
private static final String RECEIPT_TOTALPOINT_HTML_TEMPLATE = "On this rental you earned <EM>%s frequent renter points<P>";
public String htmlStatement(String name) {
Enumeration<Rental> rentals = this.rentals.elements();
String result = String.format(RECEIPT_HEADER_HTML_TEMPLATE, name);
while( rentals.hasMoreElements() ) {
Rental targetRental = (Rental) rentals.nextElement();
// 一つのレンタルに対する料金を計算する
double thisAmount = targetRental.calcTargetRentalAmount();
// この貸し出しに関する数値の表示
result += String.format(RECEIPT_DETAIL_HTML_TEMPLATE, targetRental.getMovie().getTitle(), String.valueOf(thisAmount) );
}
result += String.format( RECEIPT_TOTALAMOUNT_HTML_TEMPLATE, String.valueOf(totalAmount) );
result += String.format( RECEIPT_TOTALPOINT_HTML_TEMPLATE, String.valueOf(frequentREnterPoints) );
return result;
}
}
→こんな感じで簡単に追加できます。ただ、これはかなり不真面目です。本当にやるなら、PrinterクラスをAbstractクラスでつくって、単純な文字列で出力するStringPriterみたいなクラスとhtmlで出力するHtmlPrinterみたいなクラスを作成して、抽象化したいです。
今回のケースで行くとstatementメソッドはそのままで、RECEIPT_HEADER_TEMPLATEなどの4種類のテンプレートを差し替えるだけで動くはずなので、StringPriterの保持するTEMPLATEとHtmlPrinteの保持するTEMPLATEだけ変更すればよいですね。（すみません、実装まではしていないです。。）
というわけで、私の変更でもhtmlStatementメソッドを容易に実装できるということから、そんなに独力リファクタリングの方も、間違いではないと言えるでしょう。
最後に、書籍では料金計算の条件文をポリモルフィズムに置き換えていました。ここは、ちょっと驚愕でした。ちょっと無理やりすぎるかなぁと。。。
まず、ポリモルフィズム化する前に、背景として「①ビデオの種類毎に料金を計算する必要がある」というところと、「②裏事情として、ビデオの種類が増えそうな要望がある」という記載がありました。
②のようなケースは当然あるだろうなっておいう感覚もありますし、①のswitch caseが羅列してあったのは気持ち悪かったというのも事実です。
この部分について、私が導き出した回答としては、MovieTypeというenumに料金情報を持たせてその計算方法はRentalクラスが知っているという形式にしました。
一方で書籍は、Movieクラスに料金計算まで実施させるようにしています。また、Movieクラスで計算する際に、新たにPriceクラスを抽象クラスとして定義し、そのサブクラスとしてChildrensPrice、RegularPrice、NewReleasePriceというクラスを実装するようにしていました。
ここについては、私は反対でした。なぜならばChildrensPrice、RegularPrice、NewReleasePriceは、設定値が異なるだけで計算ロジック自体は同じであると考えているからです。当然計算ロジックが追加された場合のためにこのような継承関係にしておくのはよいのですが、ChildrensPriceとRegularPriceとNewReleasePriceは、今回私が示したように、「基本料金+延長料金（何日を過ぎると延長になるかはChildren・Regular・NewReleaseで異なる）」というロジックで計算することができます。
まとめ
とまぁ、最初の内は同じような変更をしていましたが、最後は全く書籍とは違うといった結果になりました。とはいえ、独力リファクタリングと書籍のリファクタリングとの差分は、独力リファクタリングが完全に劣っていると思える部分はなく、むしろ独力リファクタリングも捨てたもんじゃないなと思ってみたりしています。（自画自賛？？）
とはいえ、プロの思考プロセスと自分のプロセスを並べて比較してみるというのは、非常に面白かったですし、勉強になりました。引き続き、この書籍を読み進めて理解を深めたいと思います。
もし、リファクタリング力をつけようと思っている方は、まずは、本稿で最初に示した基ネタをベースに、まずは独力でリファクタリングを実施してみることをお勧めいたします。
コーディングと同じで、リファクタリング結果も、それぞれの見え方によって違ってくるのでその違いの善し悪しを議論できたりすると楽しそうですね。