コピペコードと共通化とどちらがましか?
コピペコードと共通化とどちらがましか?
結論から言うと、「出来の悪い共通化 <<<< コピペして書き換え <<<< 出来のいい共通化」です。状況によってはもっといい方法もあります。
まずはコピペで
とある会社では業務に必要なものを貸し出しています。入退室カードとか、工具とか、車両とか、携帯電話とかさまざまです。これらをコンピューターで管理することにしました。
まずは入退室カードから
ひとまず入退室カードの貸し出しだけ実装して利用を始めることにしました。入退室カードはなくしたら大変なので、誰にどの番号のカードを貸したのかまで追跡します。また、カードを貸したらカードを管理している総務部にメールで通知することになっています。
void 入退室カードの貸出(従業員番号)
{
//# 空いている入退室カードの番号を調べる
//
// カード貸出台帳から借りられていないカード番号を取り出す
//
// ... 長いコード
var カード番号 = // ...
//# カードを貸し出し中にする
//
// カード貸出台帳に貸すカードの番号と貸す相手の従業員番号を記録する
//
// ... 長いコード
//# 貸出記録を付ける
//
// 貸出台帳に貸すカードの番号と貸す相手の従業員番号と、貸し出したのがカードだということを記録する
//
// ... 長いコード
//# 貸し出したことを総務にメールで通知する
//
// 貸すカードの番号と貸す人の従業員番号と合わせて総務にメールする
//
// ... 長いコード
}
長いですねぇ... あたりまえですけど、プロダクションコードはもっともっと長くなります。
// で始まる行には実際にはコードが入っていると思ってください。//# で始まる行がコメント行です。
次に工具セットの貸し出し
このシステムで工具セットの貸し出しにも対応することにしました。工具と言っても特殊なものでもなく、量も多いため、総務にメールをしたり、貸し出し数を把握したり、個体を管理したりしません。その代わり毎週月曜日の朝に工具セットの数を確認し、数が減ってたら紛失したとみなして工具セットを追加購入する運用にしています。
入退室カードより単純なのでコピペで簡単に作れます。
void 工具セットの貸出(従業員番号)
{
// //# 空いている入退室カードの番号を調べる
// //
// // カード貸出台帳から借りられていないカード番号を取り出す
// //
// // ... 長いコード
// var カード番号 = // ...
//# カードを貸し出し中にする
//
// 工具貸出台帳に工具セットを貸す人の従業員番号を記録する
//
// ... 長いコード
//# 貸出記録を付ける
//
// 貸出台帳に貸す相手の従業員番号と、貸し出したのがカードだということを記録する
//
// ... 長いコード
// //# 貸し出したことを総務にメールで通知する
// //
// // 貸すカードの番号と貸す相手の従業員番号と合わせて総務にメールする
// //
// // ... 長いコード
}
コピペでやる人はこういう特徴があるのではないでしょうか? (偏見です)
- 元のコードから不要なコードを削らずにコメントにして残す
- 元のコードのコメントも修正が必要だけど、修正されないことが多い
もっともっと!
このシステムでは車両や携帯電話などの様々なアイテムの種類への対応を進めていきます。それ自体でコピペコードで問題が出ることはあまりありません。
コピペコードで怖いのは、共通の貸出の枠組みに仕様変更があったときです。
- 貸す相手の従業員番号だけでなく、貸し出した担当者の従業員番号も記録したい
- 貸す相手が従業員以外にも対応しないといけなくなった (アイテムごとに違う)
10 種類のアイテムの対応していた場合、それぞれ個別に修正しなければなりません。そんなの簡単だと思う方も多いと思うんですけど、結構大変なんです。
- 本当に 10 種類なのかどうか (長い時間たつと、メインの開発者が抜けてていて分からなくなっていることや、仕様書などのドキュメントと実体の剥離が大きくなります)
- コピペで作られているはずなんだけど、それぞれで修正方法が大きく異なり、どこをどう修正すればいいかがアイテムの種類ごとに考えないといけない
まあ、簡単に説明すると、修正ミスを引き起こしやすいのです。このような修正を何度も何度も行うと、より各アイテムの種類ごとのコードの差が大きくなり、修正の難易度を押し上げます。
(自分の観測範囲ではこのような問題が起きないよう、横展開した後にきれいにそろうよう整理しているコードを見たことがありません)
出来の悪い共通化
何度も横展開で修正されたコピペコードの修正とか大変です。ミスを起こしやすいのに、ミスをすると怒られるという...
というわけで、共通化しよう! ってなるわけです。
void 貸出(アイテムの種類, 従業員番号)
{
string カード番号;
if (アイテムの種類 == "入退室カード")
{
//# 空いている入退室カードの番号を調べる
//
// カード貸出台帳から借りられていないカード番号を取り出す
//
// ... 長いコード
カード番号 = // ...
}
if (アイテムの種類 == "入退室カード")
{
//# カードを貸し出し中にする
//
// カード貸出台帳に貸すカードの番号と貸す相手の従業員番号を記録する
//
// ... 長いコード
}
//# 貸出記録を付ける
//
string 識別番号;
if (アイテムの種類 == "入退室カード")
{
識別番号 = カード番号;
}
else if (アイテムの種類 == "工具セット")
{
識別番号 = "";
}
// 貸出台帳に貸す相手の従業員番号とアイテムの種類と識別番号を記録する
//
// ... 長いコード
if (アイテムの種類 == "入退室カード")
{
//# 貸し出したことを総務にメールで通知する
//
// 貸すカードの番号と貸す人の従業員番号と合わせて総務にメールする
//
// ... 長いコード
}
}
不吉な匂いがしませんか? まだ二種類のアイテムしか対応していませんので何とかなりますが、どんどんアイテムの種類を足していくと、どんどん長く複雑になっていきます。
この貸出メソッドは「貸し出す担当者も記録する」くらいなら楽に修正できるかもしれませんが、「従業員以外にも貸し出す」に対応しようとすると結構厄介です。それでもコピペコードよりもましかもしれません。
本当に怖いのは、各アイテムごとの仕様変更です。
- 入退室カードに、会議室しか入れない「ゲストカード」を増やしたい
- 工具セットもちゃんと借りれるか調べたい (けど、個体は追わない)
入退室カードの仕様変更による修正で、他の工具セットに影響を与えないように注意して修正しなければなりません。まだ入退室カードと工具セットだけしか対応していないのでましですが、たくさんのアイテムの種類に対応し、何度も仕様変更された後のコードだと、相当注意してもバグが出ます。
関数はそのうち数千行を超えると思われますが、入退室カードの貸出に関係があるのは 100 行くらいでしょう。本来はその 100 行くらいを読めば済むはずなのですが、度重なる修正で、分岐やネストが増加し、変数を他と共有し、関数の引数も共有し、どんどん入り乱れ、数千行のコードを注意深く読まないと、関係がないと判断するのがほぼ不可能になります。(なんとなく関係ないなら可能)
例えば、ゲストカードに対応させるため、次のように関数の宣言を修正します。
void 貸出(アイテムの種類, 従業員番号, ゲストカード = false)
この調子で引数を宣言すると、将来引数が 100 くらいになってもおかしくありません。そこでよく使われるのが引数の共有です。例えば、車両の貸出に対応させる際にゲストカード引数を外部レンタカーか社が所有しているかのフラグに利用するのです。こうなると、ゲストカード引数を使っているけど入退室カード用ではないと判断するのがより難しくなります。
このような些細に見えるかもしれない問題も数千行レベルでやられるとかなり厳しくなります。多分、一ヶ月くらいかければできると思うのですが、そんな余裕をもらえることはまれだと思います。
このような状況では、入退室カードの仕様変更の修正で、入退室カード以外に影響がないことを保証できません。ということは、入退室カードの修正で他もすべて再テストをする必要があります。入退室カードと工具セットだけならまだいいですが、10 種類に対応させていたら、単純計算でテスト工数が 10 倍になります。
それでもコピペよりまし?
プロダクトの開発では、コーディングの工数より、テストやそれに付随するデバッグをはじめとする修正作業の工数の方が大きいです。
コピペコードは共通の仕様の変更による横展開に工数がかかるのは間違いありませんが、出来の悪い共通化でも結局のところすべてのアイテムの種類のテストが必要で、テスト工数は大きくは変わりません。(おそらく、発見されるバグはコピペコードの方が多いと思われるので、共通化されていた方が少ないとは思われます) 思ってるほど工数の削減にはならないのです。
それよりも、新しいアイテムの種類に対応させたり、各アイテムの種類の仕様変更をしたりする際に、すべての種類のテストが必要になる出来の悪い共通化の方法は、コピペコードより悪いと言えます。
一般的に、共通部分への仕様変更よりも個別の仕様変更や追加の方が圧倒的に大きく、コピペの方がましだった状態になります。
出来のいい共通化
整理します。
- コピペコードは共通の仕様変更に弱い
- 出来の悪い共通化は個別の仕様へ高に弱い
ということになります。
どちらを取るかを考えるのではなく、どちらの問題も解消するのが出来のいい共通化です。
出来の悪い共通化では、共通化と言いつつ、各アイテムごとの貸し出しの仕様を関数が知っていました。結果、どのような仕様変更でもこの関数を修正することになり、大変なことになったわけです。
共通関数はあくまで共通の処理だけにとどめ、各アイテムの種類ごとの仕様はそのアイテムの方で何とかしてもらうというのが基本です。方法はいくつかありますが、目的を達成できればどれでも構いません。
関数を渡す
LL を触っている人なら関数を関数に渡すというのをよくやると思います。
貸出(
アイテムの種類,
従業員番号,
識別番号を取り出す関数,
アイテムの種類ごとの台帳へ記録する関数,
通知をする関数
)
{
var 識別番号 = 識別番号を取り出す関数();
アイテムの種類ごとの台帳へ記録する関数(識別番号, 従業員番号);
貸出台帳に記録する(従業員番号, アイテムの種類, 識別番号);
通知をする関数(従業員番号, アイテムの種類, 識別番号);
}
入退室カードではこのようにそれぞれの実装を関数で書いて利用します。
string 空いているカード番号を調べる()
{
// ...
}
入退室カード貸出台帳へ記録(従業員番号, 識別番号)
{
// ...
}
総務へ通知(従業員番号, アイテムの種類, 識別番号)
{
// ...
}
貸出(
"入退室カード",
"A012B",
空いているカード番号を調べる,
入退室カード貸出台帳へ記録,
総務へ通知
);
工具セットは次のようになります。
string 何もしない()
{
return "";
}
工具セット貸出台帳へ記録(従業員番号, 識別番号)
{
// ...
}
通知をしない()
{
}
貸出(
"工具セット",
"A012B",
何もしない,
工具セット貸出台帳へ記録,
通知をしない
);
もし、入退室カードの貸し出しに仕様変更があったらどうでしょうか? コピペコードと同じく、他のアイテムの種類への影響はありません。テスト工数まで含めてもコピペコードと同じくらいしか工数がかかりません。
もし、共通の貸し出しに仕様変更があったらどうでしょうか? 例えば、貸し出しをした従業員の従業員番号も記録する、のようなものです。さすがに共通関数の実装だけを修正するだけとはいかないので、すべてのアイテムの種類に対して修正をする必要があります。それでも出来の悪い共通化よりテスト工数まで含めた工数がかかることはないでしょう。
つまり、多くの仕様変更・追加において、コピペコードや出来の悪い共通化のどちらよりも、最悪でも同程度の工数しかかからないということです。
他の方法
関数を渡す方法も悪くはないのですが、今回のケースのように渡す関数が多くなると、ややこしい感じが出てきます。ですので、テンプレートメソッドパターンやストラテジーパターンを利用する方が一般的でしょう。
別解
出来のいい共通化で解決した! というとそうでもないこともあります。今回の様々なアイテムの貸出はまさにそういったケースです。
適当に扱える工具セット、紛失が大問題になる入退室カード、足りなくなれば外部レンタカーから借りてくる車両など、それぞれ求めているものが大きく異なります。正直、共通の貸出台帳に記録する以外はほとんど共通点はありません。無理やり共通化をすると、新しいアイテムの種類や仕様変更のたびに、共通部分も修正されることになります。
これを防ぐには抽象化をより進めるしかありません。
- 各アイテムの種類ごとの貸出
- 共通の貸出台帳への記録
- 後始末
くらいまで抽象化すれば、仕様へ鋼や追加で共通部分への修正が必要になることはなくなるとは思います。しかし、抽象化を進めた結果、各アイテムの種類ごとの流れをつかむのが難しくなっています。
じゃあ、コピペってこと? ではありません。流れを記述する関数と、実際に何をするかの関数に分けます。
入退室カードの貸出(従業員番号)
{
var カード番号 = 空いているカード番号を調べる();
入退室カード貸出台帳へ記録(従業員番号, カード番号);
貸出台帳に記録する(従業員番号, "入退室カード", カード番号); // これは共通関数
総務へ通知(従業員番号, アイテムの種類, カード番号)
}
工具セットの貸出(従業員番号)
{
工具セット貸出台帳へ記録(従業員番号);
貸出台帳に記録する(従業員番号, "工具セット", ""); // これは共通関数
}
多分、コードがきれいな人ってのはこういうのをさくっと書ける人なんでしょうねぇ。
気を付けること
共通化したらコピペの問題が防げるとかそんなことはありません。よりよくするための技法はたくさんあり、それらをうまく活用して問題を解決していきます。方法を変えたら他の問題が出てしまって変わらないとかよくあります。
自分が特に気を付けているのが次の二つです。
- モジュール化
- 凝集度
モジュール化
モジュール化というのは、関連する処理に名前を付けて整理することです。最小単位は関数ですが、関連する関数をまとめてクラスや名前空間などに配置して、整理を続けます。
出来の良い共通化の技法を採用しても、何が共通なのかの分析に失敗したらもうお手上げです。プロダクトへの理解が足りないうちは外すのがあたりまえですし、たとえうまくいったとしても、ビジネスの変化により失敗になるケースもあります。
モジュール化で、共通部分を取り出したり、逆に排除したり、こういった組み換えを容易にします。もちろん、テストは必要なので、気楽にできることではありませんが、それでも組み換えが容易なのは間違いありません。
凝集度
凝集度と言っても色々あるのですが、ここでは論理的凝集を特に気を付けます。
出来の悪い共通化の例では、関数のコードが数千行に対して実際に実行されるコードは 100 行くらいになると書きました。この状態が論理的凝集です。
アイテムの種類が入退室カードか工具セットかで、実行されるコードが全然違います。実行されないコードも多いです。理想は、引数によらず関数のすべてのコードが実行されることです。すべての関数がこの理想通りに書けるわけではありませんが、特に仕様変更や追加が予想される区分での分岐は避けるべきです。
あちこちから呼ばれる関数だけで構いませんので、関数内で実際に実行される行数を大雑把に確認してみてください。引数の値によるぶれが大きかったり、60% 程度だとしたら共通化は失敗していると思ってください。共通の処理なのに、区分 (各業務) ごとの処理が含まれています。修正のたびにどんどん論理的凝集が進みます。
修正が頻発する論理的凝集な関数は典型的な共通化の失敗です。発見したらなるはやで対処することをお勧めします。
うまくできていれば、例えば入退室カードの機能をシステムから除去するとき、不要なコードを取り除くのが簡単になります。指標としてわかりやすいので、この機能を除去した際に不要なコードを簡単に取り除けるのか? を実際にはやらないにしても、考えて設計するようにしています。
最後に
自分はプログラマーなので、コピペの横展開で何度も痛い目にあって、だから共通化だ! で今度は別の問題で痛い目を見てます。
- 安易に共通化しないこと。それよりもモジュール化が大事。共通化は後でもできる。
- 共通化の名のもとに論理的凝集を生み出さないこと。かえって状況が悪化します。
Discussion