RevokeMsgPatcherのソースを眺める①～適当リファクタ～
今週のお相手 RevokeMsgPatcher
https://github.com/huiyadanli/RevokeMsgPatcher
例のごとく、なにをどうするものなのか知らないけど。
コードお借りします。
コードメトリクスを計測
50を切ってるようなものほとんどないんだけど。すごい。
AppModifier.csがその中でもやんちゃっぽいので、今回のターゲットはこのファイルにする。
プロジェクト: RevokeMsgPatcher
構成: Debug
スコープ: 型
アセンブリ: D:\git_refact\RevokeMsgPatcher\RevokeMsgPatcher\bin\Debug\RevokeMsgPatcher.exe
名前空間: RevokeMsgPatcher.Modifier
型: AppModifier
保守容易性指数: 69
サイクロマティック複雑度: 43
継承の深さ: 1
クラス結合: 14
コード行: 71
ひとまず
ざっとみ。abstractクラスで、コメントはたぶん中国語？なので、全くあてにできない。
英語ならギリギリ単語が拾えるのに。義務教育のパワーを感じる。
テストはなさそう。というか全クラスない。abstractのテスト書くより、実装側で書いたほうがよさそうなんだけど。
300行ぐらいですごくコンパクト。クラスの行数の許容範囲ってどれぐらいなんだろう。
IsAllFilesExist
/// <summary> /// 判断APP安装路径内是否都存在要修改的文件 /// </summary> /// <param name="installPath">APP安装路径</param> /// <returns></returns> public bool IsAllFilesExist(string installPath) { if (string.IsNullOrEmpty(installPath)) { return false; } int success = 0; foreach (TargetInfo info in config.FileTargetInfos.Values) { string filePath = Path.Combine(installPath, info.RelativePath); if (File.Exists(filePath)) { success++; } } if (success == config.FileTargetInfos.Count) { return true; } else { return false; } }
ネストもそこまで深くないし、行数も短い。ほえー大体処理わかったし、変えるとこないのでは。
・メンバ変数のFileTargetInfosからpathを生成。
・Fileの存在確認。
・全件存在しているか。
メソッド名とも動作はかわらないし。しいてあげるなら、この成功件数を数える部分はメソッド化してもいいのかなぁ。
やりすぎ感もあるけど、テスト書いてやって見よう。
File.Exists関数をだます。
テストを書くのはいいけど、微妙に面倒な奴がある。
「if (File.Exists(filePath))」
こいつ。こいつに使われているFile.Existsは実際に引数のパスが実行環境にあるかを返却する関数だ。
問題は、テスト環境では実際のFileの有無なんて興味はなくて、この関数の返却値でどうするか。が興味のある部分。
visualStdioにはShimなる凶悪な仕組みがあるが、ライセンス的に無料版のVisualStdioでは使用できない。
こーゆーときはどうすればいいのだろうか。実際にわざわざファイルを配置するのは失敗テスト間違いなしなので。
こまったら「レガシーコード改善ガイド」に頼る。
困ったときの本だのみ。このへんかなぁ。使えると思う。直接使わずにラップしてしまおう。
API を ラップ する 手法 では、 可能 な 限り そっくり に API を 模 し た インタフェース を 用意 し て、 その API の ラッパー を 作り ます。
間違い を 最小限 に する ため に、 シグネチャ の 維持（ 328） を 行い ます。 API を ラップ する 手法 の 優位 点 の 1 つ は、 最終 的 に API コード への 依存 を 排除 できる こと です。
マイケル・C・フェザーズ. レガシーコード改善ガイド (Kindle の位置No.5256-5260). 翔泳社. Kindle 版.
うさんくさいコードになったが許してもらおう。テストがかけないんだ。
/// <summary> /// 判断APP安装路径内是否都存在要修改的文件 /// </summary> /// <param name="installPath">APP安装路径</param> /// <returns></returns> public bool IsAllFilesExist(string installPath) { if (string.IsNullOrEmpty(installPath)) { return false; } int success = 0; foreach (TargetInfo info in config.FileTargetInfos.Values) { string filePath = Path.Combine(installPath, info.RelativePath); if (IsFileExists(filePath)) { success++; } } if (success == config.FileTargetInfos.Count) { return true; } else { return false; } } protected virtual bool IsFileExists(string filePath) { return File.Exists(filePath); }
とりあえず前半のNull保証
[TestMethod]
public void Test_IsAllFilesExist01()
{
var obj = new FakeAppModifier();
bool actual;
actual = obj.IsAllFilesExist(null);
Assert.AreEqual(false, actual);
actual = obj.IsAllFilesExist(string.Empty);
Assert.AreEqual(false, actual);
}
ファイル有無の基本動作
public class FakeAppModifier : AppModifier { public override string FindInstallPath(){ throw new NotImplementedException(); } public override string GetVersion(){ throw new NotImplementedException(); } public bool _isFileExist { get; set; } protected override bool IsFileExist(string IsFileExists) { return _isFileExist; } private int key; public void AddTargetInfo(TargetInfo info) { config.FileTargetInfos = new Dictionary<string, TargetInfo>() { { key++.ToString(), info } }; } } public void Test_IsAllFilesExist02() { var obj = new FakeAppModifier(); bool actual; obj.AddTargetInfo(new TargetInfo()); obj._isFileExist = false; actual = obj.IsAllFilesExist("dummy"); Assert.AreEqual(false, actual); obj._isFileExist = true; actual = obj.IsAllFilesExist("dummy"); Assert.AreEqual(true, actual); }
Fakeで無理やりExistさせる。うーん。これなんのテストしとるかわからんけど・・・
Combine要素を足すべきか。複数来た場合のbool値の切り替えもできない。これはよくない。
[TestClass]
public class AppModifierTest
{
public class FakeAppModifier : AppModifier
{
private FakeAppModifier() { }
public override string FindInstallPath(){
throw new NotImplementedException();
}
public override string GetVersion(){
throw new NotImplementedException();
}
private int called;
protected override bool IsFileExist(string filePath)
{
string path = Path.Combine(_args[called].Item1, _args[called].Item2);
called++;
return filePath == path;
}
private List<Tuple<string, string>> _args;
private int _key;
private string _installPath;
public static FakeAppModifier InitializeAddTarget(string installPath)
{
FakeAppModifier obj = new FakeAppModifier()
{
_key = 0,
_args = new List<Tuple<string, string>>(),
_installPath = installPath,
config = new App() { FileTargetInfos = new Dictionary<string, TargetInfo>() },
};
return obj;
}
public void AddTargetInfoNotExist(string relativePath)
{
// 引数保存
_args.Add(new Tuple<string, string>("", relativePath));
config.FileTargetInfos.Add(_key++.ToString(), new TargetInfo() { RelativePath = relativePath });
}
public void AddTargetInfo(string relativePath)
{
// 引数保存
_args.Add(new Tuple<string, string>(_installPath, relativePath));
config.FileTargetInfos.Add(_key++.ToString(), new TargetInfo() { RelativePath = relativePath });
}
}
[TestMethod]
public void Test_IsAllFilesExist01()
{
var obj = FakeAppModifier.InitializeAddTarget("");
bool actual;
actual = obj.IsAllFilesExist(null);
Assert.AreEqual(false, actual);
actual = obj.IsAllFilesExist(string.Empty);
Assert.AreEqual(false, actual);
}
[TestMethod]
public void Test_IsAllFilesExist02()
{
string installPath = "dummy";
var obj = FakeAppModifier.InitializeAddTarget(installPath);
bool actual;
string relativePath = "dummy_relative";
obj.AddTargetInfo(relativePath);
actual = obj.IsAllFilesExist("dumm");
Assert.AreEqual(false, actual);
obj = FakeAppModifier.InitializeAddTarget(installPath);
obj.AddTargetInfo(relativePath);
actual = obj.IsAllFilesExist(installPath);
Assert.AreEqual(true, actual);
}
[TestMethod]
public void Test_IsAllFilesExist03()
{
string installPath = "dummy";
var obj = FakeAppModifier.InitializeAddTarget(installPath);
bool actual;
string relativePath1 = "dummy_relative1";
string relativePath2 = "dummy_relative2";
obj.AddTargetInfo(relativePath1);
obj.AddTargetInfo(relativePath2);
actual = obj.IsAllFilesExist(installPath);
Assert.AreEqual(true, actual);
obj = FakeAppModifier.InitializeAddTarget(installPath);
obj.AddTargetInfo(relativePath1);
obj.AddTargetInfo(relativePath2);
obj.AddTargetInfoNotExist("dummy");
actual = obj.IsAllFilesExist(installPath);
Assert.AreEqual(false, actual);
}
}
だいぶ無理やりくさい。ループ、API（APIじゃないけど）、DB接続がメソッドに絡みだすとテストを書くのが極端にしんどくなっていくな。
ひとまずこれでリファクタできる。
/// <summary> /// 判断APP安装路径内是否都存在要修改的文件 /// </summary> /// <param name="installPath">APP安装路径</param> /// <returns></returns> public bool IsAllFilesExist(string installPath) { if (string.IsNullOrEmpty(installPath)) { return false; } foreach (TargetInfo info in config.FileTargetInfos.Values) { string filePath = Path.Combine(installPath, info.RelativePath); if (!IsFileExist(filePath)) { return false; } } return true; } protected virtual bool IsFileExist(string filePath) { return File.Exists(filePath); }
件数なんて数える必要ないじゃん。ということでごっそり削った。
グループ名: AppModifierTest
グループ化: Hierarchy
グループの完全名: AppModifierTest
期間: 0:00:00.008383
0 件のテストが失敗しました
0 件のテストがスキップされました
3 件のテストが合格しました
結果 1 の名前: Test_IsAllFilesExist01
結果 1 の成果: 成功
結果 1 の継続期間: 0:00:00.0000892
結果 1 のスタック トレース:
結果 1 のメッセージ:
結果 1 の標準出力:
結果 1 の標準エラー:
結果 2 の名前: Test_IsAllFilesExist02
結果 2 の成果: 成功
結果 2 の継続期間: 0:00:00.0001158
結果 2 のスタック トレース:
結果 2 のメッセージ:
結果 2 の標準出力:
結果 2 の標準エラー:
結果 3 の名前: Test_IsAllFilesExist03
結果 3 の成果: 成功
結果 3 の継続期間: 0:00:00.008178
結果 3 のスタック トレース:
結果 3 のメッセージ:
結果 3 の標準出力:
結果 3 の標準エラー:
再測定する。
プロジェクト: RevokeMsgPatcher
構成: Debug
スコープ: 型
アセンブリ: D:\git_refact\RevokeMsgPatcher\RevokeMsgPatcher\bin\Debug\RevokeMsgPatcher.exe
名前空間: RevokeMsgPatcher.Modifier
型: AppModifier
保守容易性指数: 69
サイクロマティック複雑度: 43
継承の深さ: 1
クラス結合: 14
コード行: 70
ほかに期待して今回はよしとする。
