2dust/V2rayNのソースコードを読んでみた（～勝手にリファクタリングする～）
目次
拝借したコード
github.com
ありがとうございます。お借りして勉強させていただきます。
また、こちらは上記のソースコードを詳細に説明したり、使用法に言及した記事ではないため、
そのような記事をお求めの方は別の記事を参照ください。ここにはありません。
ライセンス
github.com
GPLのようです。コピーレフトということで、今回一部解析して書き直しを行いますが、
そのソースコードについても同様のライセンスとしてここに宣言します。
いざ。
環境
VisualStdio2017 Community
Windows10
ひとまず対象メソッドの選定。
今回はC#のソースコードなので、VisualStudioのコードメトリクス計算機能を使うと客観的に数値を拾うことができます。便利
gyazo.com
ということでbtoOK_Clickメソッドを対象とします。
イベント系の操作は初めは簡潔に書けるんですが、よくよく見ると長大になっていく傾向がありますね。このメソッドがそうなってしまったのかは知りませんが。
メソッドを俯瞰する。
btnOK_clikc
private void btnOK_Click(object sender, EventArgs e) { try { Process[] existing = Process.GetProcessesByName("v2rayN"); foreach (Process p in existing) { string path = p.MainModule.FileName; if (path == GetPath("v2rayN.exe")) { p.Kill(); p.WaitForExit(100); } } } catch (Exception ex) { // Access may be denied without admin right. The user may not be an administrator. showWarn("Failed to close v2rayN(关闭v2rayN失败).\n" + "Close it manually, or the upgrade may fail.(请手动关闭正在运行的v2rayN，否则可能升级失败。\n\n" + ex.StackTrace); } StringBuilder sb = new StringBuilder(); try { if (!File.Exists(fileName)) { if (File.Exists(defaultFilename)) { fileName = defaultFilename; } else { showWarn("Upgrade Failed, File Not Exist(升级失败,文件不存在)."); return; } } string thisAppOldFile = Application.ExecutablePath + ".tmp"; File.Delete(thisAppOldFile); string startKey = "v2rayN/"; using (ZipArchive archive = ZipFile.OpenRead(fileName)) { foreach (ZipArchiveEntry entry in archive.Entries) { try { if (entry.Length == 0) { continue; } string fullName = entry.FullName; if (fullName.StartsWith(startKey)) { fullName = fullName.Substring(startKey.Length, fullName.Length - startKey.Length); } if (Application.ExecutablePath.ToLower() == GetPath(fullName).ToLower()) { File.Move(Application.ExecutablePath, thisAppOldFile); } string entryOuputPath = GetPath(fullName); FileInfo fileInfo = new FileInfo(entryOuputPath); fileInfo.Directory.Create(); entry.ExtractToFile(entryOuputPath, true); } catch (Exception ex) { sb.Append(ex.StackTrace); } } } } catch (Exception ex) { showWarn("Upgrade Failed(升级失败)." + ex.StackTrace); return; } if (sb.Length > 0) { showWarn("Upgrade Failed,Hold ctrl + c to copy to clipboard.\n" + "(升级失败,按住ctrl+c可以复制到剪贴板)." + sb.ToString()); return; } Process.Start("v2rayN.exe"); MessageBox.Show("Upgrade successed(升级成功)", "", MessageBoxButtons.OK, MessageBoxIcon.Information); Close(); }
うーん。ながい。およそ100Stepあります。クレイジーなネストは見受けられないので、それほど回収するのは困難ではないかもしれません。
プロセスを切る処理
try { Process[] existing = Process.GetProcessesByName("v2rayN"); foreach (Process p in existing) { string path = p.MainModule.FileName; if (path == GetPath("v2rayN.exe")) { p.Kill(); p.WaitForExit(100); } } } catch (Exception ex) { // Access may be denied without admin right. The user may not be an administrator. showWarn("Failed to close v2rayN(关闭v2rayN失败).\n" + "Close it manually, or the upgrade may fail.(请手动关闭正在运行的v2rayN，否则可能升级失败。\n\n" + ex.StackTrace); }
管理者権限がなければタスクの終了に失敗することがある。ということでしょうか。ここはおそらく処理をするのに都合の悪いプロセスを断ち切っている処理と思われます。
引数が一つも使われていないので、このメソッドはもうそのまま切り出してしまっても問題ないでしょう。
わざわざ次のブロックとの間に改行もあるところから、この処理が以降の処理と分離していることはこのソースコードを作った人も気づいていたことだと思います。
気になるのはExceptionは吐きますが、移行の処理は実行されてしまうところです。おそらくになりますが、returnで処理を中断すべきところに見えます。
考慮して、このように変形してみました。
private bool Killv2RayN() { try { Process[] existing = Process.GetProcessesByName("v2rayN"); foreach (Process p in existing) { string path = p.MainModule.FileName; if (path == GetPath("v2rayN.exe")) { p.Kill(); p.WaitForExit(100); } } } catch (Exception ex) { // Access may be denied without admin right. The user may not be an administrator. showWarn("Failed to close v2rayN(关闭v2rayN失败).\n" + "Close it manually, or the upgrade may fail.(请手动关闭正在运行的v2rayN，否则可能升级失败。\n\n" + ex.StackTrace); return false; } return true; } private void btnOK_Click(object sender, EventArgs e) { if (!Killv2RayN()) return;
Kill2RayNメソッドの中身はある程度シンプルな形に保たれているので、これ以上刻むのは置いておき、次に移ります。
fileNameのセットアップ処理
StringBuilder sb = new StringBuilder(); try { if (!File.Exists(fileName)) { if (File.Exists(defaultFilename)) { fileName = defaultFilename; } else { showWarn("Upgrade Failed, File Not Exist(升级失败,文件不存在)."); return; } }
ファイルの存在確認処理でしょうか。特に問題もないように見えますが・・・ややIf文の辺りがさらっと読み飛ばすには引っかかりを覚えます。
おっと。ここのshowWarnではreturnをしているので、やはり先ほどのException処理はreturnを忘れていたのでしょう。
こちらも引数は使用していないので、メソッドに切り出すことができるかもしれません。
また、この処理をtryの中でやる必要も見えてきません。外側でやってしまっても十分に機能するのではないでしょうか。
private bool ExistsUpdateFile() { if (File.Exists(fileName)) { return true; } if (File.Exists(defaultFilename)) { fileName = defaultFilename; return true; } showWarn("Upgrade Failed, File Not Exist(升级失败,文件不存在)."); return false; }
まずは早期return で逃げ切りました。ここでこのメソッドはExistsを行っているのではなく、fileNameのセットアップを行っているのだと気づきました。
メソッド名は妥当なものに変更します。
private bool SetupFileName() { if (File.Exists(fileName)) { return true; } if (File.Exists(defaultFilename)) { fileName = defaultFilename; return true; } showWarn("Upgrade Failed, File Not Exist(升级失败,文件不存在)."); return false; } private void btnOK_Click(object sender, EventArgs e) { if (!Killv2RayN()) return; if (!SetupFileName()) return; StringBuilder sb = new StringBuilder(); try { 。。。 }
このような形に変更しました。幾分、見通しが良くなったと思います。ここでまた一つ気付きます。
tryを行う直前のif分の判定はこれから行う処理のセットアップを行っている。セットアップに失敗した場合は処理を中断したい。きっとそのようなことに見えます。
せっかくなのでメソッドに切り出して表現しておきます。
private bool SetupOKClickProcess() { if (!Killv2RayN()) return false; if (!SetupFileName()) return false; return false; } private void btnOK_Click(object sender, EventArgs e) { if (!SetupOKClickProcess()) return; StringBuilder sb = new StringBuilder();
これは冗長な対応かもしれません。SetupOKClickProcess側の処理はやや見にくいものになってしまいました。
ゴミFileの削除処理
StringBuilder sb = new StringBuilder(); try { string thisAppOldFile = Application.ExecutablePath + ".tmp"; File.Delete(thisAppOldFile); string startKey = "v2rayN/";
startKey の宣言と、その上の部分に特別な関係はなさそうです。また、fileDeleteを含む処理はこれまで見てきた処理と同様これから実行するメインロジックの前準備に相当していると考えます。
少し位置を調整してみましょう。DeleteFileはメソッドに切り出し、前処理の仲間に加えます。
startKeyの生成位置はTryの中でやる必要はなく、説明変数であったり、メソッド内定数に相当する立ち位置です。ので、setup処理の直後に宣言することにしました。
private bool DeleteTemporaryFile() { try { string thisAppOldFile = Application.ExecutablePath + ".tmp"; File.Delete(thisAppOldFile); } catch (Exception ex) { showWarn("Upgrade Failed(升级失败)." + ex.StackTrace); return false; } return true; } private bool SetupOKClickProcess() { if (!Killv2RayN()) return false; if (!SetupFileName()) return false; if (!DeleteTemporaryFile()) return false; return false; } private void btnOK_Click(object sender, EventArgs e) { if (!SetupOKClickProcess()) return; string startKey = "v2rayN/"; StringBuilder sb = new StringBuilder(); try { using (ZipArchive archive = ZipFile.OpenRead(fileName)) {
さて、ここで困ったことが発生しました。参照を適当に確認していたためにDeleteFile時に生成していた「thisAppOldFile」が以降の処理で使用されているのに気付くことができませんでした。
thisAppOldFileは
string thisAppOldFile = Application.ExecutablePath + ".tmp";
となっています。この生成であれば、コンストラクタ生成時に決定することでさしたる問題はなさそうです。あまりメンバ変数が増えてしまうのもどうかと思いますが・・・メンバ変数に移管しましょう。
private readonly string thisAppOldFile = Application.ExecutablePath + ".tmp";
こんなかんじで。
メインロジック
using (ZipArchive archive = ZipFile.OpenRead(fileName)) { foreach (ZipArchiveEntry entry in archive.Entries) { try { if (entry.Length == 0) { continue; } string fullName = entry.FullName; if (fullName.StartsWith(startKey)) { fullName = fullName.Substring(startKey.Length, fullName.Length - startKey.Length); } if (Application.ExecutablePath.ToLower() == GetPath(fullName).ToLower()) { File.Move(Application.ExecutablePath, thisAppOldFile); } string entryOuputPath = GetPath(fullName); FileInfo fileInfo = new FileInfo(entryOuputPath); fileInfo.Directory.Create(); entry.ExtractToFile(entryOuputPath, true); } catch (Exception ex) { sb.Append(ex.StackTrace); } } }
usingもforeachもある程度どうしようもありませんが・・・一度メソッドに全文切り出します。
private bool Upgrade() { string startKey = "v2rayN/"; StringBuilder sb = new StringBuilder(); try { using (ZipArchive archive = ZipFile.OpenRead(fileName)) { foreach (ZipArchiveEntry entry in archive.Entries) { try { if (entry.Length == 0) { continue; } string fullName = entry.FullName; if (fullName.StartsWith(startKey)) { fullName = fullName.Substring(startKey.Length, fullName.Length - startKey.Length); } if (Application.ExecutablePath.ToLower() == GetPath(fullName).ToLower()) { File.Move(Application.ExecutablePath, thisAppOldFile); } string entryOuputPath = GetPath(fullName); FileInfo fileInfo = new FileInfo(entryOuputPath); fileInfo.Directory.Create(); entry.ExtractToFile(entryOuputPath, true); } catch (Exception ex) { sb.Append(ex.StackTrace); } } } } catch (Exception ex) { showWarn("Upgrade Failed(升级失败)." + ex.StackTrace); return false; } if (sb.Length > 0) { showWarn("Upgrade Failed,Hold ctrl + c to copy to clipboard.\n" + "(升级失败,按住ctrl+c可以复制到剪贴板)." + sb.ToString()); return false; } return true; }
これでは単にややこしいのが移動しただけです。内部を見ていきます。実はこれと言って直せるところが・・・
というわけにはいかないのですね。ただ、この辺りであればとくに影響もなく修正もできてしまえるので、よい塩梅かもしれません。
ぱっと見問題がなさそうなので、各処理に名前を付けることができないか見ていきます。
if (entry.Length == 0) { continue; }
後続の処理をみるかぎり、entryFullNameに対して処理を行うので判定しているようです。ただこれがやらないとエラーになるからやっているのか、
単に不要な処理だからやっているのか判断に迷うところです。迷ったときはコメントを置いておくほうがよさそうです。今回は省略します。
string fullName = entry.FullName; if (fullName.StartsWith(startKey)) { fullName = fullName.Substring(startKey.Length, fullName.Length - startKey.Length); }
ここの処理は・・・fullNameで宣言していますが、その後内容が書き換わっています。fullNameなのかfullNameではない何かなのか。
判断に迷うところです。こういった情報は使用する側で考慮するのが面倒な場合が多いので、fullNameの取得をentry側に持たせて悩むのをやめてみます。
どうにもここ以外で使用箇所がなさそうなので、ZipArchiveEntryの拡張クラスで対応してみます。
ざっとこのような感じに
using System.IO.Compression; namespace v2rayUpgrade { public static class ZipArchiveEntryExtensions { public static string FullName(this ZipArchiveEntry instance, string startKey) { string fullName = instance.FullName; if (fullName.StartsWith(startKey)) { fullName = fullName.Substring(startKey.Length, fullName.Length - startKey.Length); } return fullName; } } }
使用側はnamespaceもおなじなので特にUsingの定義もなく
try { if (entry.Length == 0) { continue; } string fullName = entry.FullName(startKey); if (Application.ExecutablePath.ToLower() == GetPath(fullName).ToLower()) { File.Move(Application.ExecutablePath, thisAppOldFile); } string entryOuputPath = GetPath(fullName); FileInfo fileInfo = new FileInfo(entryOuputPath); fileInfo.Directory.Create(); entry.ExtractToFile(entryOuputPath, true); } catch (Exception ex) { sb.Append(ex.StackTrace); }
少し見通しがよくなりましたか？次いきます。
string entryOuputPath = GetPath(fullName); FileInfo fileInfo = new FileInfo(entryOuputPath); fileInfo.Directory.Create(); entry.ExtractToFile(entryOuputPath, true);
ここも一連の処理ですが、ゴニョゴニョしてExtractToFileしたいってのが本音だろうな。と思います。こちらも先ほど作った拡張クラスに放り込んでみましょう。
public static void ExtractToFile(this ZipArchiveEntry instance, string path) { FileInfo fileInfo = new FileInfo(path); fileInfo.Directory.Create(); instance.ExtractToFile(path, true); }
こんなかんじに・・・どうも無理やり感がでてきました。fullNameも別処理で作ったFullNameがあるはずです。
おそらく、このArchiveEntryを操作するクラスを作ったほうがよいでしょう。今回はあきらめます。
対応後の確認
さてひとしきり対応が終わったので変更箇所を見てみます。
/// <summary> /// 宣言位置はおかしいが、コード説明する際にわかりやすいのでこの位置に宣言。 /// </summary> private readonly string thisAppOldFile = Application.ExecutablePath + ".tmp"; private bool SetupFileName() { if (File.Exists(fileName)) { return true; } if (File.Exists(defaultFilename)) { fileName = defaultFilename; return true; } showWarn("Upgrade Failed, File Not Exist(升级失败,文件不存在)."); return false; } private bool DeleteTemporaryFile() { try { File.Delete(thisAppOldFile); } catch (Exception ex) { showWarn("Upgrade Failed(升级失败)." + ex.StackTrace); return false; } return true; } private bool SetupOKClickProcess() { if (!Killv2RayN()) return false; if (!SetupFileName()) return false; if (!DeleteTemporaryFile()) return false; return false; } private bool Upgrade() { string startKey = "v2rayN/"; StringBuilder sb = new StringBuilder(); try { using (ZipArchive archive = ZipFile.OpenRead(fileName)) { foreach (ZipArchiveEntry entry in archive.Entries) { try { if (entry.Length == 0) { continue; } string fullName = entry.FullName(startKey); if (Application.ExecutablePath.ToLower() == GetPath(fullName).ToLower()) { File.Move(Application.ExecutablePath, thisAppOldFile); } entry.ExtractToFile(GetPath(fullName)); } catch (Exception ex) { sb.Append(ex.StackTrace); } } } } catch (Exception ex) { showWarn("Upgrade Failed(升级失败)." + ex.StackTrace); return false; } if (sb.Length > 0) { showWarn("Upgrade Failed,Hold ctrl + c to copy to clipboard.\n" + "(升级失败,按住ctrl+c可以复制到剪贴板)." + sb.ToString()); return false; } return true; }
どうでしょうか。見通しがよくなったような。悪くなったような。自己満足で終わるのもよろしくないので再度コードメトリクスを計算します。
gyazo.com
btoOK_Clickの保守容易性指数はずいぶん回復しました。
メソッドは分割しましたがクラス全体の指数も改善しているので、悪い部分が移動した。ということでもないということがわかります。
よかったよかった。めでたしめでたし。
