この記事は「リファクタリングでシステムを安定化！日頃の取り組みや工夫を教えてください by カオナビ Advent Calendar 2022の12/18分の記事となります
リファクタリングの課題
リファクタリング皆さんやってますかー！？
もちろん、私はやってません（爆）
いや、やれたらイイとは思うんですよ。本当に。
でも、リファクタリングって以下の観点でやるタイミング難しいですよね
- リリース後、基本的にコードを変更しない
- 不具合の解消や新規機能改善ではないので、それがなぜ必要なのかの経営層からの理解が薄い
- なにかあると怖い
- 面倒くさい
こういう時、理由で挙げられる最後の項目が大抵本音なわけで、結局のところ、「面倒くさい」に尽きるんだと思います。
もちろん、コードの改善が面倒くさいのもあるし、説明するのも面倒くさいというのもあります。あぁ、面倒くさい
という訳で、リファクタリングの労力に関する部分だけでも最小化したく思います
リファクタリングの基本、おさらい
リファクタリングとはなんぞや？ということを定義づけるために、今回はミノ駆動本を参考に箇条書してみます
非常に学びがある本なのでおすすめです。
- ネストを解消し、見通しをよくする
- 意味のある単位にロジックをまとめる
- 条件を読みやすくする
- ベタ書きロジックを目的を表すメソッドに置き換える
リファクタリング対象となるサンプルコードはこちらになります
class PurchasePointPayment {
final CustomerId customerId;
final ComicId comicId;
final PurchasePoint consumptionPoint;
final LocalDateTime paymentDateTime;
PurchasePointPayment(final Customer customer, final Comic comic){
if(customer.isEnabled()){
customerId = customer.id;
if(comic.isEnabled()){
comicId = comic.id;
if(comic.currentPurchasePoint.amount <= customer.processionPoint.amount){
consumptionPoint = comic.currentPurchasePoint;
paymentDateTime = LocalDateTime.now();
}
else{
throw new RuntimeException("所持ポイントが不足しています");
}
}else{
throw new IllegalArgumentException("現在取り扱いのできないコミックです");
}
}else{
throw new IllegalArgumentException("有効な購入者ではありません");
}
}
}
chatGPT～、リファクタリングしてよ～（○えもん風に）
1回目
class PurchasePointPayment {
final CustomerId customerId;
final ComicId comicId;
final PurchasePoint consumptionPoint;
final LocalDateTime paymentDateTime;
PurchasePointPayment(final Customer customer, final Comic comic){
if(!customer.isEnabled()){
throw new IllegalArgumentException("有効な購入者ではありません");
}
if(!comic.isEnabled()){
throw new IllegalArgumentException("現在取り扱いのできないコミックです");
}
if(comic.currentPurchasePoint.amount > customer.processionPoint.amount){
throw new RuntimeException("所持ポイントが不足しています");
}
customerId = customer.id;
comicId = comic.id;
consumptionPoint = comic.currentPurchasePoint;
paymentDateTime = LocalDateTime.now();
}
}
お、なるほど。
まず、1のネストを解消する件に関してはバッチリですね！
ちなみに、元々はタイプミスもあったのですが、こちらに関してもついでに修正してくれていました。。。
やるじゃない
2回目
class PurchasePointPayment {
final CustomerId customerId;
final ComicId comicId;
final PurchasePoint consumptionPoint;
final LocalDateTime paymentDateTime;
PurchasePointPayment(final Customer customer, final Comic comic) {
validateCustomer(customer);
validateComic(comic);
validatePurchasePoint(customer, comic);
customerId = customer.id;
comicId = comic.id;
consumptionPoint = comic.currentPurchasePoint;
paymentDateTime = LocalDateTime.now();
}
private void validateCustomer(final Customer customer) {
if (!customer.isEnabled()) {
throw new IllegalArgumentException("有効な購入者ではありません");
}
}
private void validateComic(final Comic comic) {
if (!comic.isEnabled()) {
throw new IllegalArgumentException("現愛取り扱いのできないコミックです");
}
}
private void validatePurchasePoint(final Customer customer, final Comic comic) {
if (comic.currentPurchasePoint.amount > customer.processionPoint.amount) {
throw new RuntimeException("所持ポイントが不足しています");
}
}
}
今度は、ネストの解消だけでなく、各エラー判定をvalidate関数として分離してくれています。これはなかなか良いですね。
与えているコードの範囲が限定されているので、customerクラスやcomicクラスのメソッド変更を伴うような"読みやすさ"の改善は対象外と考えれば良いのではないでしょうか（無駄に上から目線）
chatGPT～テストこれ本当に大丈夫なの～？
これが本当に問題ないのか？はやはりユニットテストが必要になります。
調子に乗って、chatGPTにテストコードも泣きついてみます
import org.junit.Test;
import static org.junit.Assert.*;
public class PurchasePointPaymentTest {
private Customer createMockCustomer(boolean isEnabled, int possessionPointAmount) {
// create and return a mock Customer object with the specified enabled status and possession point amount
}
private Comic createMockComic(boolean isEnabled, int purchasePointAmount) {
// create and return a mock Comic object with the specified enabled status and purchase point amount
}
@Test
public void testValidConstruction() {
Customer customer = createMockCustomer(true, 100);
Comic comic = createMockComic(true, 50);
PurchasePointPayment payment = new PurchasePointPayment(customer, comic);
// assert that the fields of payment are set correctly
}
@Test
public void testInvalidCustomer() {
Customer customer = createMockCustomer(false, 100);
Comic comic = createMockComic(true, 50);
try {
PurchasePointPayment payment = new PurchasePointPayment(customer, comic);
fail();
} catch (IllegalArgumentException e) {
// assert that the exception message is correct
}
}
@Test
public void testInvalidComic() {
Customer customer = createMockCustomer(true, 100);
Comic comic = createMockComic(false, 50);
try {
PurchasePointPayment payment = new PurchasePointPayment(customer, comic);
fail();
} catch (IllegalArgumentException e) {
// assert that the exception message is correct
}
}
@Test
public void testInsufficientPurchasePoint() {
Customer customer = createMockCustomer(true, 50);
Comic comic = createMockComic(true, 100);
try {
PurchasePointPayment payment = new PurchasePointPayment(customer, comic);
fail();
} catch (RuntimeException e) {
// assert that the exception message is correct
}
}
}
おお。さすがchatGPTだぜ！
成功時など、一部は手を加える必要はありますがいい感じのJUnitコードを書き出してくれました。
あれ。。。私いらなくね。。。？（危機感）
最後に
どこまで複雑なコードに対してリファクタリングやテストコードを書いてもらえるかは怪しいところではあるし、複数の要素が絡まってくると、そもそも条件として与えること自体が難しいというところはあるんだけど、取っ掛かりとしては悪くなさそう。
何よりも頭を使わなくて済んでいる。
これが、もう少しコードボリュームが大きくなっていったときに、「これって本当にあっているかな・・・？」を逐次確認しなくてはいけなくなってしまうと、かえって労力がかかり、本末転倒な事態になりかねないので注意が必要そう。
そういう意味では、先にテストコードを書かせたほうが良さそうではある。
もう一つ、大きな懸念としては、コードをchatGPTに投げてしまって大丈夫かどうか？という点だ。
いわゆる、情報のアップロードになってしまうので、業務で使用するようなコードを確認するのは注意が必要なのではないかと思う。
一方で、個人開発におけるリファクタリングの確認としてやる分には、一つのアイデアを得るという意味においても有用にも感じる。
何故ならば、個人開発ではコードをチェックしてくれる相手がいないのだから。
万能ではないにしても、これら機能などを活用しながら、少しでもGithubにクソコードを撒き散らすのをなんとか防ぐことができればと思う。
いや、面白い時代になりましたね！