概要
コードの保守性を良くするための内部設計というか、コードの書き方のテクニックを一つ紹介します。ここで言う保守性は、変更の容易さや影響の少なさです。
そういう話については、体系的かつ汎用的な法則としてまとめてある本は良著がいくつもあります。ここでは違ったアプローチとして、実務の中でよく見かけるケースについて、対象を絞って、できるだけ具体例を付けて紹介しようと思います。分厚い設計の本を読むのは辛いが、エッセンスだけでも1つずつ読めたら頭に入ってくるかも、という人などへ役に立ったら良いなと思います。
最初にポイントまとめ
クラスのメソッドが「引数で指示されないと何もできないAPI」になっていて、その呼び出し元が全てのビジネスロジックを知って引数を与えてあげる。こういうコードは保守性が悪いからやめよう。クラスのメソッドは、自分でビジネスロジックの知識を持っていて、指示されたら自分で判断して動くようにしよう。という話です。
知っている人には、「Tell, Don't ask」の原則（ちょっと変形ですが）とか、カプセル化の話だ、といえばなんとなくイメージは伝わると思います。
説明
割とよく見かけるコードとして、メソッドの動作が呼び出し元から渡される引数で全て決まるので、渡している引数を読まないとメソッドの動作が分からない、というのがあります。こういう奴です。
機能を公開するためのAPIの設計なら、これでも良いと思います。しかし、ビジネスロジックを処理する部分がこうなっていると、保守性が大きく下がります。
まず、いま触れたように、コードを読むのが大変です。動きを理解するために読む範囲が広ければ、それだけでも保守しづらいです。加えて変更の影響も大きく、呼び出し先クラスを使う機能を変更した場合、高確率で呼び出し元も大きな変更が入ってしまいます。
そうした問題を起こさないようにするには、呼び出し元が細かく判断して処理するのではなく、呼び出し元は指示だけして呼び出し先が判断するように実装すれば起きづらくなります。こんな感じです。
こういう説明だけだと抽象的で分かりづらいですね。短いコードで書くのは難しいですが、ちょっと頑張って具体例を出してみます。
具体例
悪い例:最初のコード
ファイル名が決まった「キーファイル」というものが有り、それをダウンロードする処理とします。悪い例で書くと、こういう感じになります。
void main()
{
//実際には設定ファイルやUIなどから別途読み込むと思いますが、サンプルを小さくするためハードコーディングとしています
var filePath = @"\\server\share\KeyFile.bin";
var userName = "username";
var passWord = "password";
var data = KeyFileDownloader.Download(filePath, userName, passWord);
}
class KeyFileDownloader
{
public static byte[] Download(string filePath, string userName, string passWord)
{
//filePathからダウンロード。認証はuserNameとpassWordを使う
}
}
Downloadメソッドは、汎用的に使えるAPIのようになっています。汎用性はありますが、ビジネスロジックの知識を持っていません。そのため、呼び出し元のコードも合わせて読む事で、初めて「キーファイルをダウンロードしているのだ」という意味が分かります。
悪い例:機能を追加
さて、このコードは共有フォルダからファイルをダウンロードしています。クラウドストレージに対応する必要が出てきました。クラウドストレージはUserName/Passwordではなく、認証トークンでアクセスします。それを実装してみましょう。
悪い例のような書き方をしている場合、「クラウドストレージに対応した新規パラメータを、mainが与えてあげなければ」という発想になり、「Downloadメソッドの引数を増やそう」となりがちです。すると、こうなるでしょうか。
void main()
{
//実際には設定ファイルやUIなどから別途読み込むと思いますが、サンプルを小さくするためハードコーディングとしています
var serverType = ServerType.ShareFolder;
if (serverType == ServerType.ShareFolder)
{
//こっちは共有フォルダ用の設定。userNameとpassWordが使われる
var filePath = @"\\server\share\KeyFile.bin";
var userName = "username";
var passWord = "password";
var data = KeyFileDownloader.Download(filePath, userName, passWord, null);
}
else
{
//こっちはクラウドストレージ用の設定。UserTokenが使われる
var filePath = @"https://storageaccount.com/container/KeyFile.bin";
var userToken = "44KI44GP44GT44KT44Gq44Go44GT44KN44G+44Gn6Kqt44KA5rCX44Gr44Gq44KK44G+44GX44Gf44Gt77yf5pyf5b6F44GV44Gb44Gm44GZ44G/44G+44Gb44KT44GM44CB54m544Gr56eY5a+G44GM6Zqg44GV44KM44Gm44GE44Gf44KK44Gv44GX44G+44Gb44KT44CC";
var data = KeyFileDownloader.Download(filePath, null, null, userToken);
}
class KeyFileDownloader
{
public static byte[] Download(string filePath, string? userName, string? passWord, string? userToken, ServerType serverType)?
{
if(serverType == ServerType.ShareFolder)
{
//ShareFolderを指定された場合は、filePathからダウンロード。認証はuserNameとpassWordを使う
}
else
{
//それ以外の場合は、CloudStorageとして処理。filePathからダウンロード。認証はuserTokenを使う
}
}
}
enum ServerType
{
ShareFolder,
CloudStorage
}
呼び出し元にも、Downloadメソッドにも、影響が出ています。さらに、「ShareFolderならばuserName/Passwordが必要で、CloudStorageならUserTokenが必要」という知識も、呼び出し元とDownloadメソッドの両方の実装に必要となっています。
改善案:最初のコード
最初にこうしていれば、問題が起きづらかったと思います。
void main()
{
KeyFileDownloader.Init();
var data = KeyFileDownloader.DownloadKeyFile();
}
class KeyFileDownloader
{
public static void Init()
{
//実際には設定ファイルやUIなどから別途読み込むと思いますが、サンプルを小さくするためハードコーディングとしています
_baseUncPath = @"\\server\share";
_userName = "username";
_passWord = "password";
}
string _baseUncPath;
string _userName;
string _passWord;
//ファイル名はKeyFileの仕様なので、外から情報を与えてもらう必要は無い
private static readonly string FileName = @"KeyFile.bin";
public static byte[] DownloadKeyFile()
{
//baseUncPath+FileNameからダウンロード。認証はuserNameとpassWordを使う
}
}
KeyFileDownloader.DownloadKeyFile()というメソッドになり、「何かをダウンロードする」という汎用APIではなく、「キーファイルをダウンロードする」という役割がはっきり分かるようになりました。呼び出し元のコードを読まなくても、このメソッドとクラス内のフィールドだけを見れば動作が理解できます。
改善案:機能を追加
同じようにクラウドストレージ対応しても、呼び出し元には影響が出ません。ServerTypeによって使うパラメータを分けるという知識も、呼び出し先のクラスの中だけに実装されていて、呼び出し元は知らずに済んでいます。
void main()
{
KeyFileDownloader.Init();
var data = KeyFileDownloader.DownloadKeyFile();
}
class KeyFileDownloader
{
public static void Init()
{
//実際には設定ファイルやUIなどから別途読み込むと思いますが、サンプルを小さくするためハードコーディングとしています
var serverType = ServerType.ShareFolder;
if (serverType == ServerType.ShareFolder)
{
_baseUncPath = @"\\server\share";
_userName = "username";
_passWord = "password";
}
else
{
_baseUrl = "https://storageaccount.com/container/";
_userToken = "44KI44GP44GT44KT44Gq44Go44GT44KN44G+44Gn6Kqt44KA5rCX44Gr44Gq44KK44G+44GX44Gf44Gt77yf5pyf5b6F44GV44Gb44Gm44GZ44G/44G+44Gb44KT44GM44CB54m544Gr56eY5a+G44GM6Zqg44GV44KM44Gm44GE44Gf44KK44Gv44GX44G+44Gb44KT44CC";
}
}
ServerType _serverType;
string _baseUncPath;
string _userName;
string _passWord;
string _baseUrl;
string _userToken;
//ファイル名はKeyFileの仕様なので、外から情報を与えてもらう必要は無い
private static readonly string FileName = @"file.txt";
public static byte[] DownloadKeyFile()
{
if (serverType == ServerType.ShareFolder)
{
//_serverTypeがShareFolderの場合は、baseUncPath+FileNameからダウンロード。認証はuserNameとpassWordを使う
}
else
{
//それ以外の場合は、CloudStorageとして処理。_baseUrl+FileNameからダウンロード。認証はuserTokenを使う
}
}
}
まとめ
クラスのメソッドについて、呼び出し元が色々判断してコントロールするのではなく、クラス内で自分で判断して処理をできるようにして、呼び出し元は指示を出すだけにした方が、保守性が良くなると思うという話でした。例を出してみましたが、上手く伝わったでしょうか。こうした設計の話を説明するのは難しいので、色々試行錯誤してみます。
免責事項、もしくは言い訳
本文中には書かなかったものの、ちょっと言い訳をしておこうというコーナーです。
- 例が極端すぎる。ちょっと関数を分けるだけで解決するような例であって、外からコントロールしているとかそういう問題ではない。
- 短いコードでこういった内部設計のメリットを伝えるのは難しく、あえて極端な例にしています。本当はもっと複雑な話だと思って読んでもらえると助かります。
- カプセル化とか責務分割のような違う話まで混じってしまっている
- 保守性の良さ・凝集度の高さのようなシンプルな原則に対して、実現するためのアプローチはいくつもあるので、1つだけ説明するコードを書くのは難しく、複数の要素が入ってしまいがちです。別のアプローチの例として読んでもらっても良いと思います。
- なんで全部staticメソッド？
- サンプルコードをできるだけ短くするためです。複数インスタンスがポイントになる話題ではないため、省略しました。
- 改善後のソースコードも良くない。もっとオブジェクト指向的に良くできる
- 説明したいアプローチに絞った改善だけをしています。話がブレてしまうので、それ以外の観点での改善はできるだけ避けています。