「良いコード／悪いコードで学ぶ設計入門」第13章 モデリング 〜 第14章 リファクタリング
13章もパラパラっとページをめくってみた感じ、文章がメインぽいですね。
13.1 邪悪な構造に陥りがちな User クラス
面白い。いかにもありそうな気がする。
- User というログインユーザーを示すクラスを作る
- そこにIDや名前などのプロパティを追加していく
- User を管理するための UserManager というクラスも作る
- さらに法人ユーザーも同じ User クラスを使い（ん？）
- 法人番号などのプロパティを追加していく（個人ユーザー使わないよね、そのプロパティ）
- 法人ユーザー を管理するための CorporationManager というクラスも作る
そして、UserMaganeger, CorporationManager 双方が User クラスを見た時・・・
- CorporationManager が個人 User の法人番号が空白としてエラー（個人ユーザーだし当然）
- UserManager が人名に利用できない
(株)などの文字を利用しているとしてエラー
これらを回避するために、分岐が増え・・・メンテナンスが難しくなり・・・おぉ、目に見えるようだ。
13.2 モデリングの考え方とあるべき構造
- モデルはシステム構造の説明のために用いる
- システムとは何か？
- システムは目的達成のための手段
- 特定の目的達成のために、最低限考慮が必要な要素を備えたものがモデル
ここで、例として通販サイトにおける商品モデルを取り上げるが、商品を構成するプロパティを全部突っ込んである巨大な商品モデルになっている。
これを、「注文時の商品モデル」「配送時の商品モデル」のように、目的ごとに定義した商品モデルにする。
13.3 よくないモデルの問題点と解決方法
上記の User モデルは、複数の目的のために無理やり利用されており、モデリングしているようでモデリングしていないといえる
うむ。
「User が持ちうるもの」という意味では一貫しているといえるけど、特定の目的のためという意味では不要なプロパティも多いよなぁ。
ここから先は文章が続き、本の要約にしかならないので、しばし飛ばします。
飛ばしつつ、気になったところを書き抜きます。
- 特定の目的に特化して設計することで、変更に強い高品質な構造になる
- モデルに目的外の要素が入り込んでいる場合、さらに見直す
思えば、自分も巨大モデルを作りたがる傾向がある気がします。
それで楽をしてきたと思うのですが、果たして本当に楽だっただろうか・・・？
うーん。
13.4 機能を左右するモデリング
- 裏に隠れた真の目的を見破る
- これは起こりうるトラブルを解決するための情報が揃えられるか？的な観点で説明してる
- 機能性をイノベートする「深いモデル」
- 目的・手段に応じた抽象化をする
- 本質的課題を解決し、機能性の確信に貢献するモデル
14.1 リファクタリングの流れ
- 外から見た挙動を変えずに、構造を整理すること
- おっと、久々に骨のありそうなコードだ。書いてみよう。
- なお、久々すぎてJavaのコードの読み方をすっかり忘れてしまっていた
そうなのか！ / とほほのJava入門 - とほほのWWW入門
— sironekotoro💙💛 (@sironekotoro) 2022年9月2日
“コンストラクタ は、クラス名と同じ名前を持つ特別なメソッドです。インスタンスが生成された時に自動的に呼ばれ、インスタンスの初期化処理を行います。” https://t.co/x8Op28bDiT- リスト14.1相当のつもりで書き始めたけど、Mouse （使ってのオブジェクト指向）の書き方に従ってたらリスト14.5相当くらいのコードになってた。
ここクリックして展開
#!/usr/bin/env perl use strict; use warnings; use feature qw/say/; package Customer { use Mouse; has name => ( is => 'ro', isa => 'Str', required => 1, ); has id => ( is => 'ro', isa => 'Int', required => 1, ); has possession_point => ( is => 'ro', isa => 'Int', required => 1, default => 1000, ); sub is_enabled { return !!1; } } package Comic { use Mouse; has id => ( is => 'ro', isa => 'Int', required => 1, ); has current_purchase_point => ( is => 'ro', isa => 'Int', default => 100, ); sub is_enabled { return !!1; } } package PurchasePointPayment { use Carp qw/croak/; use Mouse; use namespace::autoclean; use Time::Piece; # 購入者 has customer => ( is => 'ro', isa => 'Customer', required => 1, trigger => sub { croak "有効な購入者ではありません。" unless $_[1]->is_enabled; } ); # 購入するWebコミック has comic => ( is => 'ro', isa => 'Comic', required => 1, trigger => sub { croak "現在取扱できないコミックです。" unless $_[1]->is_enabled; } ); # 購入日時 has payment_date_time => ( is => 'ro', isa => 'Time::Piece', required => 0, default => sub { croak "所持ポイントが不足しています。" unless $_[0]->comic->current_purchase_point <= $_[0]->customer->possession_point; return localtime(); } ); __PACKAGE__->meta->make_immutable(); } package main; my $purchase_pointp_ayment = PurchasePointPayment->new( customer => Customer->new( name => 'sironekotoro', id => 1 ), comic => Comic->new( id => 10 ), );
- ここからは以下の改修ポイントを実装していく。
unless $_[1]->is_enabled;でも良い気がするが、if $_[1]->is_disabled;にしておく- 購入日のプロパティでの残高チェックのサブルーチンを、customer に移す
で、改修したのがこちら。
しれっと use v5.36;, use signatures 使ってます。
ここクリックして展開
#!/usr/bin/env perl
use strict;
use warnings;
use v5.36;
use feature qw/say signatures/;
package Customer {
use Mouse;
has name => (
is => 'ro',
isa => 'Str',
required => 1,
);
has id => (
is => 'ro',
isa => 'Int',
required => 1,
);
has possession_point => (
is => 'ro',
isa => 'Int',
required => 1,
default => 1000,
);
sub is_disabled {
return !!0;
}
sub is_short_of_point ( $self, $comic ) {
return !!1
if $self->possession_point <= $comic->current_purchase_point;
}
}
package Comic {
use Mouse;
has id => (
is => 'ro',
isa => 'Int',
required => 1,
);
has current_purchase_point => (
is => 'ro',
isa => 'Int',
default => 100,
);
sub is_disabled {
return !!0;
}
}
package PurchasePointPayment {
use Carp qw/croak/;
use Mouse;
use namespace::autoclean;
use Time::Piece;
# 購入者
has customer => (
is => 'ro',
isa => 'Customer',
required => 1,
trigger => sub {
my $customer = $_[1];
croak "有効な購入者ではありません。" if $customer->is_disabled;
}
);
# 購入するWebコミック
has comic => (
is => 'ro',
isa => 'Comic',
required => 1,
trigger => sub {
my $comic = $_[1];
croak "現在取扱できないコミックです。" if $comic->is_disabled;
}
);
# 購入日時
has payment_date_time => (
is => 'ro',
isa => 'Time::Piece',
required => 0,
default => sub {
my $self = $_[0];
croak "所持ポイントが不足しています。"
if $self->customer->is_short_of_point( $self->comic );
return localtime();
}
);
__PACKAGE__->meta->make_immutable();
}
package main;
my $purchase_pointp_payment = PurchasePointPayment->new(
customer => Customer->new( name => 'sironekotoro', id => 1 ),
comic => Comic->new( id => 10 ),
);
14.2 ユニットテストでリファクタリングのミスを防ぐ
- 悪魔を呼び寄せるような邪悪なコードには、テストコードが書かれていないことが多いです
ということで、リファクタリングをする前のコード書いていきます。
ここクリックして展開
#!/usr/bin/env perl use strict; use warnings; use v5.36; use feature qw/say signatures/; package Product { use Carp qw/croak/; use Mouse; use namespace::autoclean; has price => ( is => 'ro', isa => 'Int', required => 1, ); __PACKAGE__->meta->make_immutable(); } # 配送管理クラス package DeliveryManager { use Carp qw/croak/; use Mouse; use namespace::autoclean; has products => ( is => 'ro', isa => 'ArrayRef', required => 1, # trigger => sub { croak '', unless ( $_[0] ) }, ); sub delivery_charge ($self) { my $charge = 0; my $total_price = 0; for my $product ( @{ $self->products } ) { $total_price += $product->price; } if ( $total_price < 2000 ) { $charge = 500; } else { $charge = 0; } return $charge; } __PACKAGE__->meta->make_immutable(); } package main; my $deliverely_manager = DeliveryManager->new( products => [ Product->new( price => 100 ), Product->new( price => 200 ), ] ); say $deliverely_manager->delivery_charge();
まず、Manager って名前が悪いよね。何でもメソッドを放り込まれる悪魔の名前だったような。
それはさておき、（この本の）リファクタリングの仕方を追っていきます。
まず、あるべき形を決めて、そこに旧来の DeliveryManager クラスから移植していきます。
- 購入する商品一覧である、買い物かごクラス
# 買い物かご package ShoppingCart { use Carp qw/croak/; use Mouse; use namespace::autoclean; has products => ( is => 'ro', isa => 'ArrayRef[Product]', required => 0, default => sub { [] }, ); sub add ( $self, $product ) { my @adding = @{ $self->products }; push @adding, $product; return __PACKAGE__->new( products => \@adding ); } __PACKAGE__->meta->make_immutable(); }
- 商品クラス
package Product { use Carp qw/croak/; use Mouse; use namespace::autoclean; has id => ( is => 'ro', isa => 'Int', required => 1, ); has name => ( is => 'ro', isa => 'Str', required => 1, ); has price => ( is => 'ro', isa => 'Int', required => 1, ); __PACKAGE__->meta->make_immutable(); }
- 配送料を計算するクラス
package DeliveryCharge { use Carp qw/croak/; use Mouse; use namespace::autoclean; has amount => ( is => 'ro', isa => 'Int', required => 1, default => -1 ); __PACKAGE__->meta->make_immutable(); }
このあとにテストを書きます。
#!/usr/bin/env perl use strict; use warnings; use Test::Simple tests => 2; use lib qw(. ../); use Product; use ShoppingCart; use DeliveryCharge; { # 商品の合計額が2000円未満の場合、配送料は500円 my $empty_cart = ShoppingCart->new(); my $one_product_added = $empty_cart->add( Product->new( id => 1, name => '商品A', price => 500 ) ); my $two_product_added = $one_product_added->add( Product->new( id => 2, name => '商品B', price => 1499 ) ); my $charge = DeliveryCharge->new( shopping_cart => $two_product_added ); ok( $charge->amount == 500, "商品の合計金額が2000円未満の場合、配送料は500円" ); } { # 商品の合計額が2000円以上の場合、配送料は無料 my $empty_cart = ShoppingCart->new(); my $one_product_added = $empty_cart->add( Product->new( id => 1, name => '商品A', price => 500 ) ); my $two_product_added = $one_product_added->add( Product->new( id => 2, name => '商品B', price => 1500 ) ); my $charge = DeliveryCharge->new( shopping_cart => $two_product_added ); ok( $charge->amount == 0, "商品の合計金額が2000円以上の場合、配送料は0円" ); }
この時点でのファイル構成はこんな感じ。
Perl では t というディレクトリの中に拡張子 t でテストファイルを作ります。
$ tree
.
├── DeliveryCharge.pm
├── Product.pm
├── ShoppingCart.pm
├── list_14_11.pl
└── t
└── deliver_charge_test.t
で、t ディレクトリと同じ階層（t ディレクトリの中ではない）で prove コマンドを打つとテストを実行してくれます。
もちろん、t ディレクトリの中で perl deliver_charge_test.t でもok
$ prove t/deliver_charge_test.t .. 1/2 # Failed test '商品の合計金額が2000円未満の場合、配送料は500円' # at t/deliver_charge_test.t line 22. # Failed test '商品の合計金額が2000円以上の場合、配送料は0円' # at t/deliver_charge_test.t line 36. # Looks like you failed 2 tests of 2. t/deliver_charge_test.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/2 subtests Test Summary Report ------------------- t/deliver_charge_test.t (Wstat: 512 (exited 2) Tests: 2 Failed: 2) Failed tests: 1-2 Non-zero exit status: 2 Files=1, Tests=2, 0 wallclock secs ( 0.03 usr 0.01 sys + 0.06 cusr 0.01 csys = 0.11 CPU) Result: FAIL
はい、エラーが出ました。
今回は、というかテストファーストで進める場合は「あるべき構造」のガワを作り「あるべき応答」でテストをしたので、中身を作っていない以上エラーが出て当然です。
ここからリファクタリングをしていきます。
リスト14.13
まずはエラーを出さないように、最低限の実装です。
ここクリックして展開
package DeliveryCharge { use Carp qw/croak/; use Mouse; use namespace::autoclean; use List::Util; has shopping_cart => ( is => 'ro', isa => 'ShoppingCart', required => 1, ); has amount => ( is => 'ro', isa => 'Int', required => 0, builder => "_build_amount", ); sub _build_amount { my $self = shift; my $amount = 0; my $total_price = $self->shopping_cart->products->[0]->price + $self->shopping_cart->products->[1]->price; if ( $total_price < 2000 ) { $amount = 500; } else { $amount = 0; } return $amount; } __PACKAGE__->meta->make_immutable(); } 1;
リスト14.14
このままだと、テストは通るけど、商品が2つまでしか入らないショッピングカートになってしまうので、ちゃんと書きます。
ここクリックして展開
package DeliveryCharge { use Carp qw/croak/; use Mouse; use namespace::autoclean; use List::Util; has shopping_cart => ( is => 'ro', isa => 'ShoppingCart', required => 1, ); has amount => ( is => 'ro', isa => 'Int', required => 0, builder => "_build_amount", ); sub _build_amount { my $self = shift; my $amount = 0; my $total_price = 0; for my $product ( @{ $self->shopping_cart->products } ) { $total_price += $product->price; } if ( $total_price < 2000 ) { $amount = 500; } else { $amount = 0; } return $amount; } __PACKAGE__->meta->make_immutable(); } 1;
今は配送料を扱う DeliveryCharge クラス内で、商品の合計を出して配送料を決めている。
本来、商品の合計を出すのは ShoppingCart クラスのはず。
ということで、リファクタリングしたのがこちらです！
ここクリックして展開
package ShoppingCart { use v5.36; use Carp qw/croak/; use Mouse; use namespace::autoclean; has products => ( is => 'ro', isa => 'ArrayRef[Product]', required => 0, default => sub { [] }, ); sub add ( $self, $product ) { my @adding = @{ $self->products }; push @adding, $product; return __PACKAGE__->new( products => \@adding ); } sub total_price { my $self = shift; my $amount = 0; for my $product ( @{ $self->products } ) { $amount += $product->price; } return $amount; } __PACKAGE__->meta->make_immutable(); } 1;
package DeliveryCharge { use Carp qw/croak/; use Mouse; use namespace::autoclean; use List::Util; use lib qw/./; use ShoppingCart; use constant { CHARGE_FREE_THRESHOLD => 2000, PAY_CHARGE => 500, CHARGE_FREE => 0, }; has shopping_cart => ( is => 'ro', isa => 'ShoppingCart', required => 1, ); has amount => ( is => 'ro', isa => 'Int', required => 0, builder => "_builder_amount", ); sub _builder_amount { my $self = shift; my $amount = $self->shopping_cart->total_price() < CHARGE_FREE_THRESHOLD ? PAY_CHARGE : CHARGE_FREE; } __PACKAGE__->meta->make_immutable(); } 1;
いやー、正直ここの部分（リスト14.14〜リスト14.19）は本に買いてあるコードを追っても全然わからず混乱。
結局、最後のリファクタリングが終わった後のコードを見て、何をするべきなのかを理解したという感じです。
このあとはリファクタリングする時に気をつける点として「機能追加とリファクタリングを同時に行わない」とか、あやふやな仕様を理解するための分析手法として「仕様化テスト」「思考リファクタリング」といった手法が紹介されています。
